Hi Ari,Andrus:
2009/6/15 Aristedes Maniatis <[email protected]> > 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. It is userd for the CM. New added "PropertyPanel" in CM needs a Property Key List of the DataMap when "PropertyPanel" is initialized. Before discuss the CM, ti should be removed. The old patch had no this change. > > > > * 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 >
