This is great. I tried something similar a couple of years ago but got some strange behaviour in some Figs that I didn't understand at the time. I'm afraid to admit I eventually gave up.
I worried about commiting such a major change and being responsible for testing every single element in detail. If we can share that responsibility that would be great. To get such consistency is worth the effort. Some general thoughts Factory pattern. =========== I'm about to post some further comments to issue 3068. That needs some further thought and I don't want it to distract too much from your work hence just the link here rather than inline comments. Builder pattern =========== I recently picked up the new edition of Effective Java and was just flicking through the chapter headings a couple of days ago. I took a look back at item 2 - Consider a builder when faced with many constructor parameters. We don't have so many parameters but the pattern may add some flexibility for future if we have missed some other useful argument. Is there any value in this or is it over-architecting? History ===== All this is very much an educated guess from what I've seen in my time on these projects. I think GEF and ArgoUML originally followed Javabean style pattern with just empty constructors for Figs and getters/setter for each property. The result of that was something that could be easily loaded and built by reflection but it also made it difficult to enforce that the Fig was completely built at any stage. The obvious other drawback was lack of immutability. I don't know when the alternative constructors were introduced or maybe they had always been there. They were already inconsistent when I arrived. They make building the Figs a little more obvious to the developer but introduced possible inconsistencies in behaviour of drawn Figs or loaded Figs without solving the mutability problem. I'm really pleased to see this gap being closed. For GraphModel usage I think this is very historic. If I understand rightly NSUML wasn't the first UML repository we used. Jason had his own non-visual model structure built around GEF's GraphModel. I think as we have our own model repository usage of this is not needed. Whoever replaced the old repository with NSUML presumably never got round to modifying the constructor arguments. Layer ===== There were previously some problems that Michiel and I had with this but I forget the details. Looking at the current code I can't see why there would be an issue. It does look like a good candidate to add to the constructor. At the moment when a Fig is added to a layer [ LayerDiagram.add(Fig) ] it is that method that calls in turn to Fig.setLayer(Layer) so that the top level parent Fig and the layer have a many to one bidirectional link to each other. We could reverse that behaviour and have setting the layer of the fig add itself to the layer. The problem with doing that in the constructor is that the fig could only be part constructed at the time it is added to the layer and that may have some side effects. That indicates to me a good reason to introduce a factory at some stage. The factory can enforce that the Fig is only added to the layer once the Fig is fully constructed and that the two way reference is complete before the new Fig is returned to the client caller. Cheers Bob. --------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
