Pedro Rodrigues wrote:
> On Sun, Feb 28, 2010 at 12:35 PM, Michael Vehrs <Michael.Burschik@...> wrote:
>> Our XML (de)serialization code could do with some cleaning up.
>> Can we reduce the number of methods related to serialization, such as
>> toXML, toXMLImpl, toXMLElement, and so on?
> On a cursory check, toXML is final, thus cannot be overrriden by other
> sub-classes implementations; there is a good reason for this, as it
> seems to make a crucial check while saving the object, which must be
> respected by all; it then calls toXMLImpl which can and must be
> implemented by each class; its a good example of the template pattern,
> and i advise against changing it.
> toXMLImpl also uses the visitor pattern, which is also a good design
> implementation, IMHO.
> We can technically move this method ( toXML) to an auxiliary class,
> but it would require changing quite a lot of calls to it, without real
> benefits, IMHO; see caveat below, though.
toXML is final in FreeColGameObject, not in FreeColObject, which is the
base class of objects that do not need a reference to Game. The check
you refer to is, of course, generally applicable, but only a few classes
(such as Unit and Settlement) have private fields that are not always
serialized. Also, all classes extending FreeColGameObjectType have a
different readFromXMLImpl method, which takes a Specification parameter.
>> Can we split off the serialization code from FreeColObject?
> Although it may seem a good idea at first glance (separation of
> responsibilities), since such code requires intimate knowledge of each
> and every object it serializes, i believe the current design, where
> each object implements toXMLImpl, that uses the visitor pattern to
> return the relevant information, without the caller having to know
> anything about the internal structure of the object.
> Im unsure exactly the reasons behind this proposal; if the reason is
> to limit the serialization to a few objects that do really need them,
> i believe a possible approach would be moving toXMLImpl to a
> FreeColSerializable interface which would be implemented by those
> classes that need them; in that case, the auxiliary method class
> mentioned above would have actual value, and allow moving toXML out of
> FreeColObject; however, all these changes would require extensive code
This is a misunderstanding: my idea was to create a base class that
implements only the serialization methods, allowing other classes to
extend this new class directly. This also helps to explain my next remark.
>> Some of the smaller classes need only this, and could do
>> without the PropertyChange and other stuff.
> I also strongly advise against this; PropertyChange is quite useful at
> such low level and for those classes that do not make use of it, the
> overhead is negligible IMHO.
> All this is based on a very brief analysis of the code, as well as a
> cursory understanding of the architecture underlying this, however;
> its quite possible im missing something.
> Im also, like mentioned before, unsure of the reasons for this
> proposal, and would appreciate some clarification.
Well, my reason is a desire for simplification. I find the
(de)serialization code rather large, complex and difficult to
understand. For example, I would consider it easier if all classes in
common.model that required serialization were to use toXML/fromXML with
appropriate parameters. I would also like to encourage the use of
superclass methods, like readChild in the FreeColGameObjectType classes.
There are also several attributes, such as name, location, owner that
could be read by a superclass in a generalized manner, I think.