On 15/6/09 6:12 PM, Andrus Adamchik wrote:
* DataMapElement ... While the name itself sounds ok, this implies that the DataMap itself can't be a DataMapElement. In 3.0 we do have some simple code generation capabilities for DataMap, so adding properties to the DataMap seems appropriate. So maybe come back to the idea of MappingObject? (also see the next point).
I take your point, but I still think "DataMapElement" is more descriptive. Every class in Java is an Object, so using that word as part of the name conveys nothing, except that it is sort of a generic multi-purpose thing. At least DataMapElement is clear and to the point. And there is no reason why an Element can be used to describe theDataMap itself. Slightly odd, but not too confusing.
* Making AbstractQuery extend from DataMapElement may or may not be ok, but note that not all queries that can be mapped extend AbstractQuery.
Ah, why not? Shouldn't we have a rigorous class hierarchy so that common code can be kept in superclasses where they belong?
In fact I'd like to get rid of this inheritance going forward... So looks like the whole DataMapElement/MappingObject should be an interface anyways.
I don't have the experience you do in creating java libraries meant to be used by lots of people in different ways, but I can't see how moving to interfaces for this one thing will help. If interfaces are to be used, then perhaps the ultimate goal is that every 'data map element' should be only an interface, allowing a user to swap in any implementation they want without having to inherit from our classes. If not, then I think this half way mixture of inheritance and interfaces is confusing. There is already a mixture of the two I find sometimes disconcerting.
* DataMap: private List<String> propertyKeyList - I don't understand this one, and it is encoded in the XML. Before we add that to the schema, could you please explain why it should be there?
I think I see what Jack might have done there. That looks like it was used for the CM and should be removed. I'll take a look before it is committed.
* Patch for the schema includes the entire file. It is not clear what was changed there.
Most of it :-). The indenting changed because we used XSD inheritance to describe the way the property can be attached to the parent class, matching the Java hierarchy.
* DataMapElement.Property inner class... Why is this an inner class? It is exposed via public methods to the end users, so let's make it a standalone class.
I thought inner class made sense here. The naming is convenient and clearer: "DataMapElement.Property" rather than "DataMapElementProperty" shows the fact that it is intrinsically part of DataMapElement and not relevant as a "Property" outside of that usage. This is just about naming and style, and the decision based on what 'looked' right from the perspective of a user seeing this class. You originally wanted just a Map<String,String> of properties rather than a whole class, but I thought that this was a way to capture more information in the future: properties might have other attributes such as whether they travel to the client in ROP. But really, they are just a slightly enhanced Map, so an inner class seemed to capture that idea.
Also if there is a notion of properties order in the element (is there?), I guess the property should be stored in the list. Do we care about properties ordering or simply displaying them in an alphabetic order is ok? I'd say alphabetic is ok. Any scenarios when the order might be important?
I don't think order is important, so the CM can display it in whatever order makes sense.
* Property.getHolder() is not used anywhere. Let's not add API we don't need. Also since we have lots of DataMapElements in the DataMap, and only some will have properties, let's use lazy initialization of the map to save some memory.
Sure.
I also have some notes on the Modeler, but I suggest we settle on the core framework approach first, and do incremental patches. It is much easier to review and discuss things in small manageable pieces.
Fair enough. If Jack is still around he might like to do the tidying up, otherwise I'll look after it. Ari Maniatis
