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
>>
>>
>>
>>
>>
>>
>
>
>
