Hi Chris, Not sure if I would see the benefit in an abstract class here as you can’t implement anything there anyway. The way I do I in ADS for example:
Deserialization: Let the Class parse itself from the Buffer: https://github.com/apache/incubator-plc4x/blob/master/plc4j/protocols/ads/src/main/java/org/apache/plc4x/java/ads/protocol/Ads2PayloadProtocol.java#L164 <https://github.com/apache/incubator-plc4x/blob/master/plc4j/protocols/ads/src/main/java/org/apache/plc4x/java/ads/protocol/Ads2PayloadProtocol.java#L164> Call the factory method with all parsed classes: https://github.com/apache/incubator-plc4x/blob/master/plc4j/protocols/ads/src/main/java/org/apache/plc4x/java/ads/protocol/Ads2PayloadProtocol.java#L167 <https://github.com/apache/incubator-plc4x/blob/master/plc4j/protocols/ads/src/main/java/org/apache/plc4x/java/ads/protocol/Ads2PayloadProtocol.java#L167> One note as you mentioned: I don’t parse complex objects this way, only the parts of the object. Serialization: https://github.com/apache/incubator-plc4x/blob/master/plc4j/protocols/ads/src/main/java/org/apache/plc4x/java/ads/api/commands/AdsReadResponse.java#L81 <https://github.com/apache/incubator-plc4x/blob/master/plc4j/protocols/ads/src/main/java/org/apache/plc4x/java/ads/api/commands/AdsReadResponse.java#L81> So the only abstract method I define here is `public abstract AdsData getAdsData()`. These classes however are just internal to the protocol. They won’t get exposed to the plc4j-api (besides you could with proprietary send). Maybe it worth looking at these to get a better feeling for the different variants. Sebastian > Am 17.08.2018 um 14:53 schrieb Christofer Dutz <[email protected]>: > > Hi all, > > I wouldn't call it a best practice ... usually I would call it a best > practice to not mix model and logic, but this is a special case as the only > reason the model exists, is to allow exactly one serialized form. > Especially here at codecentric most people I asked definitely would go down > path 2. I'm more the pragmatic guy so I favor path 3 as well. > > So how do we want to do this? I would suggest defining a type "ProtocolModel" > (Interface or abstract class) in "driver-base". > Serialization should be simple, all we need would be a "toXyz" method that > has a ByteBuf passed in. > I would suggest to call it "public void toProtocol(ByteBuf in)". > Deserialization is a little tricky, as we sometimes need to read part of the > next elements model in order do decide which type to instantiate. > Here I would try peeking in the buffer, instantiating in the parent component > or model element and then to have the constructor parse the buffer to fill > itself. > As with an interface I can't enforce the existence of a constructor with a > certain signature, I would opt for an abstract base class. > > If we want to keep the immutable state of the model classes, the properties > should be set in the constructor. I wouldn't like to have fake immutable > classes by just implementing getters. > > When serializing I usually don't have any problems with context, however when > parsing sometimes I need additional information. > In the S7 protocol for example, in order to know how to parse the payload, I > need to know some of the parameters passed in the same request. Here I could > simply override the constructor and pass in whatever I need. > > So I would suggest this base class: > > public abstract class ProtocolModel { > > public ProtocolModel(ByteBuf in) { > } > > public abstract void toProtocol(ByteBuf out); > > } > > What do you think? Does this suit your needs? > > > Chris > > > > > Am 17.08.18, 14:12 schrieb "Julian Feinauer" <[email protected]>: > > +1 from my side as well. > I think it is definitely best practice to do it that way as it increases > encapsulation in a sense. From my perspective serialization is in this > situation part of the model not a common concern as usual. > One think I observed in the S7 Implementation was the usage of several > "instanceof" which is, from my perspective, a direct consequence of this > division. > > Julian > > Am 17.08.18, 10:51 schrieb "Sebastian Rühl" > <[email protected]>: > > Sure +1 > > Sebastian > >> Am 17.08.2018 um 10:48 schrieb Christofer Dutz <[email protected]>: >> >> Oh ... ok ... I was already preparing for a discussion :-) >> >> Well if we decide to generally go down that path, I would suggest that we >> decide on some standard conventions (interfaces in base-driver) so we do >> things the same way in all modules. >> Would you agree? >> >> Chris >> >> >> Am 17.08.18, 10:43 schrieb "Sebastian Rühl" >> <[email protected]>: >> >> Just a small note: In ADS it is implemented already this way (Option 3) >> >> Sebastian >> >>> Am 17.08.2018 um 10:38 schrieb Christofer Dutz <[email protected]>: >>> >>> Hi all, >>> >>> Yesterday I have worked hard on implementing the feature of detecting the >>> type of S7 as part of the connection process. >>> Fortunately this was a lot simpler than expected as it turned out that >>> Siemens documents the structure of the SZL (Engl. SSL) List elements in the >>> handbooks of their devices. >>> >>> But it did bring up something I noticed when last working on the S7 driver. >>> >>> I implemented the driver keeping in mind the good concepts of software >>> design and tried to follow them as good as possible. >>> One of these rules is not to couple logic and model. >>> >>> As a result of this, we have the model classes (Driver internal model) and >>> in the corresponding “XyzProtocol” class the logic for serializing and >>> deserializing. >>> >>> This worked well until I started implementing the automatic fragmentation >>> and simulation in the S7MessageProcessors. >>> >>> Now I needed little helpers that were able to estimate the size of the >>> serialized form based on the model itself. >>> >>> The result of this, is that now we have at minimum 3 places, which are >>> highly coupled that need maintaining and testing. >>> >>> This did make things tricky when implementing the SSL List processing and >>> did blow up the size and complexity of the S7Driver class. >>> >>> Now to my proposal: >>> >>> In general I see 3 options for doing this: >>> >>> >>> 1. (The current way) We have model classes without any logic, A driver >>> implementation that contains the serialization/deserialization logic, Some >>> helpers tightly linked >>> 2. For each model item, we create a separate class which contains the >>> logic for serialization/deserialization and size estimation >>> 3. We add serialization/deserialization to the model classes >>> >>> I currently favor option 3. Here my explanation why: >>> >>> Well option 1 was clean in the beginning, but I am seeing more and more >>> problems with maintaining it. >>> >>> If we went for option 2, we would also have to implement a set of factory >>> classes, that help finding the serialization/deserialization class for a >>> given input. This can be tricky especially when using inheritance. Also >>> does it make things a little more complicated when digging into the code >>> for new people. >>> >>> Option 3 would have everything related to a protocol element in one place. >>> We would simply allow calling “toProtocol()”, “fromProtocol()”, >>> “getSizeInBytes()”. This does mix model and logic, but in this case I would >>> more treat it as additional form of serialization (after all each Object in >>> java has “toString()” and the a little hidden serialization that allows >>> serializing an object to a byte stream. This would simply add another >>> option to that list. And after all the serialization/deserialization >>> doesn’t really do much logic. Another downside would be that in order to >>> know what type of object is coming, I have to shift a little knowledge to >>> the upstream object. So the S7Message has to know which types of Parameter >>> could come, instantiate the correct one and then have it fill itself with >>> the data. Thinking more about it, I would probably add a second constructor >>> to each model type, taking in the Netty ByteBuffer and implement a private >>> “fromProtocol” method as a sort of convention. And the only reason the >>> model exists, is to provide this one single from of serialization. So the >>> argument that “but we could add a different serialization later on” is not >>> valid in this case as this would actually mean a new driver. >>> >>> So what do you think? >>> >>> Please don’t just fall back to fundamentalist: “You always have to do >>> something path X” or “You never should Y”. If you do please provide >>> evidence of your concerns so we can discuss them. >>> >>> In the end I want to make our code: >>> >>> 1. Easily maintainable >>> 2. Easily testable >>> 3. Easily understandable >>> >>> Chris >>> >>> >>> >>> >>> >>> >> >> >> > > > > >
