Continued Cleanup of GenericDelegator find methods
Locked
Continued Cleanup of GenericDelegator find methods
 
|
A few months ago I spent some time discussing and adding some new find methods to the GenericDelegator, namely the find, findList, and findOne methods. As part of the effort to refine and improve things in the framework in preparation for a GA binary release I took the next step on this and deprecated a bunch of the older and more redundant methods, and started the effort to cleanup some of the older code to eliminate use of various methods. There is still more of that to do, but in the GenericDelegator.java file there are now comments about the number of references (from before beginning cleanups) to each of the find methods, most of which are "on the chopping block" if you will. The goal of this effort is to trim down the GenericDelegator to a small set of the main methods. The problem with the delegator is that there are now SO many find methods it is tough to know how it should even be used. With the eliminations even if the ones remaining involve more methods they should be way easier to use because you would use the same methods over and over and there would be less variations in parameters, and hopefully way less confusion (I run into this a lot and hop back and forth between the code I'm working on and the GenericDelegator code to get things straight. Anyway, just as before I'd really like feedback on this. For example: - was I too aggressive in certain cases and did I deprecate methods that should be kept? - was I not aggressive enough in other cases and did not deprecate methods that we can get rid of? - are there any methods we could introduce that could trim things down and make them more simple or flexible? BTW, I'd eventually like to totally eliminate these deprecated methods, preferably many of them before this framework release to avoid having too much baggage going forward. Thanks, -David |
Re: Continued Cleanup of GenericDelegator find methods
|
|
as for as scope, if the delegator can be reduced down without breaking
anything, assuming refactoring of changes through-out the framework and applications, then I vote to reduce the methods to as basic as possible. Maybe a effort to check all the depreciated ones to see what they can be re-factored into would be the next step. David E Jones sent the following on 5/3/2008 3:09 AM: > > A few months ago I spent some time discussing and adding some new find > methods to the GenericDelegator, namely the find, findList, and findOne > methods. > > As part of the effort to refine and improve things in the framework in > preparation for a GA binary release I took the next step on this and > deprecated a bunch of the older and more redundant methods, and started > the effort to cleanup some of the older code to eliminate use of various > methods. > > There is still more of that to do, but in the GenericDelegator.java file > there are now comments about the number of references (from before > beginning cleanups) to each of the find methods, most of which are "on > the chopping block" if you will. > > The goal of this effort is to trim down the GenericDelegator to a small > set of the main methods. The problem with the delegator is that there > are now SO many find methods it is tough to know how it should even be > used. With the eliminations even if the ones remaining involve more > methods they should be way easier to use because you would use the same > methods over and over and there would be less variations in parameters, > and hopefully way less confusion (I run into this a lot and hop back and > forth between the code I'm working on and the GenericDelegator code to > get things straight. > > Anyway, just as before I'd really like feedback on this. For example: > > - was I too aggressive in certain cases and did I deprecate methods that > should be kept? > > - was I not aggressive enough in other cases and did not deprecate > methods that we can get rid of? > > - are there any methods we could introduce that could trim things down > and make them more simple or flexible? > > BTW, I'd eventually like to totally eliminate these deprecated methods, > preferably many of them before this framework release to avoid having > too much baggage going forward. > > Thanks, > -David > > > > > |
Re: Continued Cleanup of GenericDelegator find methods
|
|
In reply to this post by David E Jones-3
The deprecated methods shouldn't be being used anywhere now, I've gone
through and cleared them all out (unless some have been committed while I was working on it!) David, if you want to deprecate some more of the delegator's methods I'm happy to carry on. Regards Scott 2008/5/3 David E Jones <[hidden email]>: > > As part of the effort to refine and improve things in the framework in > preparation for a GA binary release I took the next step on this and > deprecated a bunch of the older and more redundant methods, and started the > effort to cleanup some of the older code to eliminate use of various > methods. > > |
Re: Continued Cleanup of GenericDelegator find methods
|
|
Scott,
thanks so much for all your hard work on this. You are carrying on a very important task for the OFBiz project. Jacopo On May 12, 2008, at 12:19 PM, Scott Gray wrote: > The deprecated methods shouldn't be being used anywhere now, I've gone > through and cleared them all out (unless some have been committed > while I > was working on it!) > > David, if you want to deprecate some more of the delegator's methods > I'm > happy to carry on. > > Regards > Scott > > 2008/5/3 David E Jones <[hidden email]>: > >> >> As part of the effort to refine and improve things in the framework >> in >> preparation for a GA binary release I took the next step on this and >> deprecated a bunch of the older and more redundant methods, and >> started the >> effort to cleanup some of the older code to eliminate use of various >> methods. >> >> |
Re: Continued Cleanup of GenericDelegator find methods
|
|
In reply to this post by Scott Gray
Wow, that's really great Scott, thanks for your help with that. I haven't looked at any other methods in the GenericDelegator class to deprecate (and eventually remove...), but I know there is still a lot of stuff in there. We could perhaps work on some of the methods that aren't really meant to be "user facing", like the cache and other methods, and move them to a sub-class like some sort of utility class. But, I don't know... I haven't thought about it too much beyond the find methods as they were a bit out of control. There are certainly other crazy things in there... Does anyone have other ideas for things here (or in other parts of the framework) to clean up? -David On May 12, 2008, at 4:19 AM, Scott Gray wrote: > The deprecated methods shouldn't be being used anywhere now, I've gone > through and cleared them all out (unless some have been committed > while I > was working on it!) > > David, if you want to deprecate some more of the delegator's methods > I'm > happy to carry on. > > Regards > Scott > > 2008/5/3 David E Jones <[hidden email]>: > >> >> As part of the effort to refine and improve things in the framework >> in >> preparation for a GA binary release I took the next step on this and >> deprecated a bunch of the older and more redundant methods, and >> started the >> effort to cleanup some of the older code to eliminate use of various >> methods. >> >> |
Re: Continued Cleanup of GenericDelegator find methods
|
|
I guess that another task in this direction is cleaning up the
Minilang code as well... for example, transform <find-by-primary-key/> into <entity-one/> etc... Jacopo On May 13, 2008, at 6:45 AM, David E Jones wrote: > > Wow, that's really great Scott, thanks for your help with that. > > I haven't looked at any other methods in the GenericDelegator class > to deprecate (and eventually remove...), but I know there is still a > lot of stuff in there. > > We could perhaps work on some of the methods that aren't really > meant to be "user facing", like the cache and other methods, and > move them to a sub-class like some sort of utility class. > > But, I don't know... I haven't thought about it too much beyond the > find methods as they were a bit out of control. There are certainly > other crazy things in there... > > Does anyone have other ideas for things here (or in other parts of > the framework) to clean up? > > -David > > > On May 12, 2008, at 4:19 AM, Scott Gray wrote: > >> The deprecated methods shouldn't be being used anywhere now, I've >> gone >> through and cleared them all out (unless some have been committed >> while I >> was working on it!) >> >> David, if you want to deprecate some more of the delegator's >> methods I'm >> happy to carry on. >> >> Regards >> Scott >> >> 2008/5/3 David E Jones <[hidden email]>: >> >>> >>> As part of the effort to refine and improve things in the >>> framework in >>> preparation for a GA binary release I took the next step on this and >>> deprecated a bunch of the older and more redundant methods, and >>> started the >>> effort to cleanup some of the older code to eliminate use of various >>> methods. >>> >>> > |
Re: Continued Cleanup of GenericDelegator find methods
|
|
I've actually been thinking of making simple-methods more consistent and using the naming style in the set operation as opposed to the old style. In other words use "field" and "to-field", and remove all the old "field-name", "map-name", "env-name" and similarly named attributes. We would remove them from the XSD (or move them all to a big comment about deprecated operations), but leave support for them in the Java files for a while so that old code would still run. This is one of the areas of OFBiz that evolved in a funny way and is backward compatible at the expense of consistency... -David On May 13, 2008, at 3:51 AM, Jacopo Cappellato wrote: > I guess that another task in this direction is cleaning up the > Minilang code as well... for example, transform <find-by-primary-key/ > > into <entity-one/> > etc... > > Jacopo > > On May 13, 2008, at 6:45 AM, David E Jones wrote: > >> >> Wow, that's really great Scott, thanks for your help with that. >> >> I haven't looked at any other methods in the GenericDelegator class >> to deprecate (and eventually remove...), but I know there is still >> a lot of stuff in there. >> >> We could perhaps work on some of the methods that aren't really >> meant to be "user facing", like the cache and other methods, and >> move them to a sub-class like some sort of utility class. >> >> But, I don't know... I haven't thought about it too much beyond the >> find methods as they were a bit out of control. There are certainly >> other crazy things in there... >> >> Does anyone have other ideas for things here (or in other parts of >> the framework) to clean up? >> >> -David >> >> >> On May 12, 2008, at 4:19 AM, Scott Gray wrote: >> >>> The deprecated methods shouldn't be being used anywhere now, I've >>> gone >>> through and cleared them all out (unless some have been committed >>> while I >>> was working on it!) >>> >>> David, if you want to deprecate some more of the delegator's >>> methods I'm >>> happy to carry on. >>> >>> Regards >>> Scott >>> >>> 2008/5/3 David E Jones <[hidden email]>: >>> >>>> >>>> As part of the effort to refine and improve things in the >>>> framework in >>>> preparation for a GA binary release I took the next step on this >>>> and >>>> deprecated a bunch of the older and more redundant methods, and >>>> started the >>>> effort to cleanup some of the older code to eliminate use of >>>> various >>>> methods. >>>> >>>> >> > |
Re: Continued Cleanup of GenericDelegator find methods
|
|
In reply to this post by David E Jones
David E Jones wrote:
> Does anyone have other ideas for things here (or in other parts of the > framework) to clean up? > > -David I have some ideas for cleaning up screen widget code, but they probably won't go over very well. ;-) -Adrian |
Re: Continued Cleanup of GenericDelegator find methods
|
|
On May 13, 2008, at 8:31 AM, Adrian Crum wrote: > David E Jones wrote: >> Does anyone have other ideas for things here (or in other parts of >> the framework) to clean up? >> -David > > I have some ideas for cleaning up screen widget code, but they > probably won't go over very well. ;-) Do you mean the code that implements the screen widget, or the XML files and such that use the screen widget? -David |
Re: Continued Cleanup of GenericDelegator find methods
|
|
I mean the model widget java code.
David E Jones wrote: > > On May 13, 2008, at 8:31 AM, Adrian Crum wrote: > >> David E Jones wrote: >>> Does anyone have other ideas for things here (or in other parts of >>> the framework) to clean up? >>> -David >> >> I have some ideas for cleaning up screen widget code, but they >> probably won't go over very well. ;-) > > Do you mean the code that implements the screen widget, or the XML files > and such that use the screen widget? > > -David > > > |
Re: Continued Cleanup of GenericDelegator find methods
|
|
Okay, what sorts of things do you want to clean up? -David On May 13, 2008, at 10:26 AM, Adrian Crum wrote: > I mean the model widget java code. > > David E Jones wrote: >> On May 13, 2008, at 8:31 AM, Adrian Crum wrote: >>> David E Jones wrote: >>>> Does anyone have other ideas for things here (or in other parts >>>> of the framework) to clean up? >>>> -David >>> >>> I have some ideas for cleaning up screen widget code, but they >>> probably won't go over very well. ;-) >> Do you mean the code that implements the screen widget, or the XML >> files and such that use the screen widget? >> -David |
Model Widget Ideas (was: Continued Cleanup of GenericDelegator find methods)
|
|
Well, I think it's the form and menu model widgets that bug me the most. I'm really looking forward to adding Ajax capability and such, but every time I look at that code I go "Ugh, what a mess."
Each widget has a set of attributes that are read into discreet model widget properties. Some attributes control the internal processing of the model widget, but many of them control some aspect of how the widget is rendered. Those are the ones the bug me the most. Those are the attributes I want to address. If I want to add a widget attribute that controls widget rendering, I have to add the new attribute to the widget's XSD, then add code to the model widget to read it in and assign it to a discreet property, then add get() and set() methods to the model widget, and then finally go to the rendering code and do something with it. If I'm writing code to render an existing attribute, I have to reverse engineer the model widget to see what property the attribute is assigned to, then figure out what get() and set() methods are used on that property (if any - some attributes seem to have no code associated with them). If I'm lucky, the get() and set() method names make logical sense - like to retrieve the "cool-attribute" attribute I would call getCoolAttribute(), but it some cases the method is called something like getWhizBang(). Some of the model widget properties are stored as String types, while others are stored as FlexibleStringExpander types. In some cases FlexibleStringExpander objects are being created for attributes that aren't used. FlexibleStringExpander has a static method to expand strings, so we don't need instances of the class in the model widgets. All model widget attributes could be stored as String objects and string expansion could be handled through FlexibleStringExpander static method calls. Here's my idea: start off with converting all model widget attribute properties as String data types. Use the FlexibleStringExpander static method call in the getXxxx(Map context) methods. Now that all of the widget attributes are stored in the model widget as property = String value, the attributes lend themselves well to being stored in a Map. So, let's add some code to the ModelWidget base class constructor to read all model widget attributes into a Map. Then add getAttribute() and setAttribute methods - including FlexibleStringExpander versions - that get the attributes from the Map. Delete the existing discreet properties and have existing get() and set() calls call the new getAttribute() and setAttribute() methods. With those changes, we can eliminate a lot of model widget properties and constructor code. We also eliminate the need for additional get() and set() methods for future attributes. With those changes, writing rendering code is a lot easier. I add the attribute to the widget's XSD, then retrieve it in the rendering code using getAttribute(). As long as the attribute doesn't need additional widget processing, I don't even have to touch the model widget code. What do you think? -Adrian David E Jones <[hidden email]> wrote: Okay, what sorts of things do you want to clean up? -David On May 13, 2008, at 10:26 AM, Adrian Crum wrote: > I mean the model widget java code. > > David E Jones wrote: >> On May 13, 2008, at 8:31 AM, Adrian Crum wrote: >>> David E Jones wrote: >>>> Does anyone have other ideas for things here (or in other parts >>>> of the framework) to clean up? >>>> -David >>> >>> I have some ideas for cleaning up screen widget code, but they >>> probably won't go over very well. ;-) >> Do you mean the code that implements the screen widget, or the XML >> files and such that use the screen widget? >> -David |
|
|
I wanted to know if there were any objections to this approach.
I did a POC on the ModelForm class and I was able to cut the amount of code in half. I'd like to get the foundational work committed soon. Basically, I'll just add some code to the ModelWidget base class to store the widget's attributes in a Map, and add getAttribute and setAttribute methods. -Adrian Adrian Crum wrote: > Well, I think it's the form and menu model widgets that bug me the most. I'm really looking forward to adding Ajax capability and such, but every time I look at that code I go "Ugh, what a mess." > > Each widget has a set of attributes that are read into discreet model widget properties. Some attributes control the internal processing of the model widget, but many of them control some aspect of how the widget is rendered. Those are the ones the bug me the most. Those are the attributes I want to address. > > If I want to add a widget attribute that controls widget rendering, I have to add the new attribute to the widget's XSD, then add code to the model widget to read it in and assign it to a discreet property, then add get() and set() methods to the model widget, and then finally go to the rendering code and do something with it. > > If I'm writing code to render an existing attribute, I have to reverse engineer the model widget to see what property the attribute is assigned to, then figure out what get() and set() methods are used on that property (if any - some attributes seem to have no code associated with them). If I'm lucky, the get() and set() method names make logical sense - like to retrieve the "cool-attribute" attribute I would call getCoolAttribute(), but it some cases the method is called something like getWhizBang(). > > Some of the model widget properties are stored as String types, while others are stored as FlexibleStringExpander types. In some cases FlexibleStringExpander objects are being created for attributes that aren't used. FlexibleStringExpander has a static method to expand strings, so we don't need instances of the class in the model widgets. All model widget attributes could be stored as String objects and string expansion could be handled through FlexibleStringExpander static method calls. > > Here's my idea: start off with converting all model widget attribute properties as String data types. Use the FlexibleStringExpander static method call in the getXxxx(Map context) methods. > > Now that all of the widget attributes are stored in the model widget as property = String value, the attributes lend themselves well to being stored in a Map. So, let's add some code to the ModelWidget base class constructor to read all model widget attributes into a Map. Then add getAttribute() and setAttribute methods - including FlexibleStringExpander versions - that get the attributes from the Map. Delete the existing discreet properties and have existing get() and set() calls call the new getAttribute() and setAttribute() methods. > > With those changes, we can eliminate a lot of model widget properties and constructor code. We also eliminate the need for additional get() and set() methods for future attributes. > > With those changes, writing rendering code is a lot easier. I add the attribute to the widget's XSD, then retrieve it in the rendering code using getAttribute(). As long as the attribute doesn't need additional widget processing, I don't even have to touch the model widget code. > > What do you think? > > -Adrian > > David E Jones <[hidden email]> wrote: > Okay, what sorts of things do you want to clean up? > > -David > > > On May 13, 2008, at 10:26 AM, Adrian Crum wrote: > >> I mean the model widget java code. >> >> David E Jones wrote: >>> On May 13, 2008, at 8:31 AM, Adrian Crum wrote: >>>> David E Jones wrote: >>>>> Does anyone have other ideas for things here (or in other parts >>>>> of the framework) to clean up? >>>>> -David >>>> I have some ideas for cleaning up screen widget code, but they >>>> probably won't go over very well. ;-) >>> Do you mean the code that implements the screen widget, or the XML >>> files and such that use the screen widget? >>> -David > > > > |
|
|
I'd still rather see this approached based on objectives, than trying to argue for or against specific changes. What is the goal of these changes? Working based on that will make the discussion and decisions much easier, more transparent, and more to the point (ie once a point is defined). Some things I can think of: 1. for anything that is getting in the way of adding AJAX capabilities, do some refactoring before adding the AJAX stuff 2. increase (and maintain!) performance; this, BTW, is the reason for having instances of FlexibleStringExpander with pre-parsed strings in them, ie to avoid re-parsing them over and over at run-time 3. reduce code size; this is an interesting objective, but not one that is a huge deal because compared to application level code and how we write that the framework level code isn't as important as it is written and maintained infrequently, but used very frequently; the REAL goal related to this (the goal behind the goal if you will) is to reduce effort to maintain the code, and that should be the goal, not reducing code size... in fact sometimes reducing code size can increase complexity such that effort to maintain is increased by the changes... -David On Jun 16, 2008, at 9:56 AM, Adrian Crum wrote: > I wanted to know if there were any objections to this approach. > > I did a POC on the ModelForm class and I was able to cut the amount > of code in half. I'd like to get the foundational work committed > soon. Basically, I'll just add some code to the ModelWidget base > class to store the widget's attributes in a Map, and add > getAttribute and setAttribute methods. > > -Adrian > > Adrian Crum wrote: >> Well, I think it's the form and menu model widgets that bug me the >> most. I'm really looking forward to adding Ajax capability and >> such, but every time I look at that code I go "Ugh, what a mess." >> Each widget has a set of attributes that are read into discreet >> model widget properties. Some attributes control the internal >> processing of the model widget, but many of them control some >> aspect of how the widget is rendered. Those are the ones the bug me >> the most. Those are the attributes I want to address. >> If I want to add a widget attribute that controls widget rendering, >> I have to add the new attribute to the widget's XSD, then add code >> to the model widget to read it in and assign it to a discreet >> property, then add get() and set() methods to the model widget, and >> then finally go to the rendering code and do something with it. >> If I'm writing code to render an existing attribute, I have to >> reverse engineer the model widget to see what property the >> attribute is assigned to, then figure out what get() and set() >> methods are used on that property (if any - some attributes seem to >> have no code associated with them). If I'm lucky, the get() and >> set() method names make logical sense - like to retrieve the "cool- >> attribute" attribute I would call getCoolAttribute(), but it some >> cases the method is called something like getWhizBang(). >> Some of the model widget properties are stored as String types, >> while others are stored as FlexibleStringExpander types. In some >> cases FlexibleStringExpander objects are being created for >> attributes that aren't used. FlexibleStringExpander has a static >> method to expand strings, so we don't need instances of the class >> in the model widgets. All model widget attributes could be stored >> as String objects and string expansion could be handled through >> FlexibleStringExpander static method calls. >> Here's my idea: start off with converting all model widget >> attribute properties as String data types. Use the >> FlexibleStringExpander static method call in the getXxxx(Map >> context) methods. >> Now that all of the widget attributes are stored in the model >> widget as property = String value, the attributes lend themselves >> well to being stored in a Map. So, let's add some code to the >> ModelWidget base class constructor to read all model widget >> attributes into a Map. Then add getAttribute() and setAttribute >> methods - including FlexibleStringExpander versions - that get the >> attributes from the Map. Delete the existing discreet properties >> and have existing get() and set() calls call the new getAttribute() >> and setAttribute() methods. >> With those changes, we can eliminate a lot of model widget >> properties and constructor code. We also eliminate the need for >> additional get() and set() methods for future attributes. >> With those changes, writing rendering code is a lot easier. I add >> the attribute to the widget's XSD, then retrieve it in the >> rendering code using getAttribute(). As long as the attribute >> doesn't need additional widget processing, I don't even have to >> touch the model widget code. >> What do you think? >> -Adrian >> David E Jones <[hidden email]> wrote: Okay, what sorts of >> things do you want to clean up? >> -David >> On May 13, 2008, at 10:26 AM, Adrian Crum wrote: >>> I mean the model widget java code. >>> >>> David E Jones wrote: >>>> On May 13, 2008, at 8:31 AM, Adrian Crum wrote: >>>>> David E Jones wrote: >>>>>> Does anyone have other ideas for things here (or in other >>>>>> parts of the framework) to clean up? >>>>>> -David >>>>> I have some ideas for cleaning up screen widget code, but they >>>>> probably won't go over very well. ;-) >>>> Do you mean the code that implements the screen widget, or the >>>> XML files and such that use the screen widget? >>>> -David >> |
|
|
David,
Thanks for the reply! In my experience, the more code you write, the more code you have to maintain. Also, the more code you write, the more things can go wrong with it. As far as performance is concerned, I suppose we could put all attributes in FlexibleStringExpanders. Here is a practical demonstration of the goal: ---------------------------------------------- I want to add a new attribute called "cool-attribute" to the form widget. The attribute will be used by the rendering classes only - the form widget does not perform any processing based on the new attribute. So, I add the new attribute to the xsd. Then I have to update the widget java classes. *** Using the existing code - public class ModelForm extends ModelWidget { ... protected String coolAttribute; ... this.coolAttribute = parent.coolAttribute; ... if (this.coolAttribute == null || formElement.hasAttribute("cool-attribute")) { this.coolAttribute = formElement.getAttribute("cool-attribute"); } ... public String getCoolAttribute() { return this.coolAttribute; } ... public void setCoolAttribute(String coolAttribute) { this.coolAttribute = coolAttribute; } } public class HtmlFormRenderer extends HtmlWidgetRenderer implements FormStringRenderer { ... String coolAttribute = modelForm.getCoolAttribute(); // do something with cool attribute } *** Using the proposed change - public class HtmlFormRenderer extends HtmlWidgetRenderer implements FormStringRenderer { ... String coolAttribute = modelForm.getAttribute("cool-attribute"); // do something with cool attribute } ---------------------------------------------- And there is my point - a very simple change is being made unnecessarily complex. -Adrian David E Jones wrote: > > I'd still rather see this approached based on objectives, than trying to > argue for or against specific changes. > > What is the goal of these changes? Working based on that will make the > discussion and decisions much easier, more transparent, and more to the > point (ie once a point is defined). > > Some things I can think of: > > 1. for anything that is getting in the way of adding AJAX capabilities, > do some refactoring before adding the AJAX stuff > > 2. increase (and maintain!) performance; this, BTW, is the reason for > having instances of FlexibleStringExpander with pre-parsed strings in > them, ie to avoid re-parsing them over and over at run-time > > 3. reduce code size; this is an interesting objective, but not one that > is a huge deal because compared to application level code and how we > write that the framework level code isn't as important as it is written > and maintained infrequently, but used very frequently; the REAL goal > related to this (the goal behind the goal if you will) is to reduce > effort to maintain the code, and that should be the goal, not reducing > code size... in fact sometimes reducing code size can increase > complexity such that effort to maintain is increased by the changes... > > -David > > > On Jun 16, 2008, at 9:56 AM, Adrian Crum wrote: > >> I wanted to know if there were any objections to this approach. >> >> I did a POC on the ModelForm class and I was able to cut the amount of >> code in half. I'd like to get the foundational work committed soon. >> Basically, I'll just add some code to the ModelWidget base class to >> store the widget's attributes in a Map, and add getAttribute and >> setAttribute methods. >> >> -Adrian >> >> Adrian Crum wrote: >>> Well, I think it's the form and menu model widgets that bug me the >>> most. I'm really looking forward to adding Ajax capability and such, >>> but every time I look at that code I go "Ugh, what a mess." >>> Each widget has a set of attributes that are read into discreet model >>> widget properties. Some attributes control the internal processing of >>> the model widget, but many of them control some aspect of how the >>> widget is rendered. Those are the ones the bug me the most. Those are >>> the attributes I want to address. >>> If I want to add a widget attribute that controls widget rendering, I >>> have to add the new attribute to the widget's XSD, then add code to >>> the model widget to read it in and assign it to a discreet property, >>> then add get() and set() methods to the model widget, and then >>> finally go to the rendering code and do something with it. >>> If I'm writing code to render an existing attribute, I have to >>> reverse engineer the model widget to see what property the attribute >>> is assigned to, then figure out what get() and set() methods are used >>> on that property (if any - some attributes seem to have no code >>> associated with them). If I'm lucky, the get() and set() method names >>> make logical sense - like to retrieve the "cool-attribute" attribute >>> I would call getCoolAttribute(), but it some cases the method is >>> called something like getWhizBang(). >>> Some of the model widget properties are stored as String types, while >>> others are stored as FlexibleStringExpander types. In some cases >>> FlexibleStringExpander objects are being created for attributes that >>> aren't used. FlexibleStringExpander has a static method to expand >>> strings, so we don't need instances of the class in the model >>> widgets. All model widget attributes could be stored as String >>> objects and string expansion could be handled through >>> FlexibleStringExpander static method calls. >>> Here's my idea: start off with converting all model widget attribute >>> properties as String data types. Use the FlexibleStringExpander >>> static method call in the getXxxx(Map context) methods. >>> Now that all of the widget attributes are stored in the model widget >>> as property = String value, the attributes lend themselves well to >>> being stored in a Map. So, let's add some code to the ModelWidget >>> base class constructor to read all model widget attributes into a >>> Map. Then add getAttribute() and setAttribute methods - including >>> FlexibleStringExpander versions - that get the attributes from the >>> Map. Delete the existing discreet properties and have existing get() >>> and set() calls call the new getAttribute() and setAttribute() methods. >>> With those changes, we can eliminate a lot of model widget properties >>> and constructor code. We also eliminate the need for additional get() >>> and set() methods for future attributes. >>> With those changes, writing rendering code is a lot easier. I add the >>> attribute to the widget's XSD, then retrieve it in the rendering code >>> using getAttribute(). As long as the attribute doesn't need >>> additional widget processing, I don't even have to touch the model >>> widget code. >>> What do you think? >>> -Adrian >>> David E Jones <[hidden email]> wrote: Okay, what sorts of >>> things do you want to clean up? >>> -David >>> On May 13, 2008, at 10:26 AM, Adrian Crum wrote: >>>> I mean the model widget java code. >>>> >>>> David E Jones wrote: >>>>> On May 13, 2008, at 8:31 AM, Adrian Crum wrote: >>>>>> David E Jones wrote: >>>>>>> Does anyone have other ideas for things here (or in other parts >>>>>>> of the framework) to clean up? >>>>>>> -David >>>>>> I have some ideas for cleaning up screen widget code, but they >>>>>> probably won't go over very well. ;-) >>>>> Do you mean the code that implements the screen widget, or the XML >>>>> files and such that use the screen widget? >>>>> -David >>> > > |
|
|
On Jun 16, 2008, at 12:33 PM, Adrian Crum wrote: > Here is a practical demonstration of the goal: > ---------------------------------------------- > > I want to add a new attribute called "cool-attribute" to the form > widget. The attribute will be used by the rendering classes only - > the form widget does not perform any processing based on the new > attribute. > > So, I add the new attribute to the xsd. Then I have to update the > widget java classes. > > *** Using the existing code - > > public class ModelForm extends ModelWidget { > ... > protected String coolAttribute; > ... > this.coolAttribute = parent.coolAttribute; > ... > if (this.coolAttribute == null || > formElement.hasAttribute("cool-attribute")) { > this.coolAttribute = formElement.getAttribute("cool-attribute"); > } > ... > public String getCoolAttribute() { > return this.coolAttribute; > } > ... > public void setCoolAttribute(String coolAttribute) { > this.coolAttribute = coolAttribute; > } > } > > public class HtmlFormRenderer extends HtmlWidgetRenderer implements > FormStringRenderer { > ... > String coolAttribute = modelForm.getCoolAttribute(); > // do something with cool attribute > } > > *** Using the proposed change - > > public class HtmlFormRenderer extends HtmlWidgetRenderer implements > FormStringRenderer { > ... > String coolAttribute = modelForm.getAttribute("cool-attribute"); > // do something with cool attribute > } > > ---------------------------------------------- > > And there is my point - a very simple change is being made > unnecessarily complex. This sort of thing is exactly why for business logic we use generic structures like Maps (and GenericValue objects...) instead of custom objects for each little thing. For the framework though, even though it is more verbose those things DO have value and make things perform better (a variable access is faster than a Map lookup, especially important for things we do over and over which is mostly what the framework is), but even moreso it results in an API that is far more "self-documenting" and that can be used with a good variety of tools, etc. Also, in many cases getters and setters have a benefit because you can add logic over time, or hide complexity. While that is often not desired for business logic (which should be very transparent) for frameworks and lower level code this can be a big benefit and ultimately reduce the size and complexity of higher level code, especially application code which is the highest priority to make smaller (as opposed to the framework code). -David |
|
|
David E Jones wrote:
> On Jun 16, 2008, at 12:33 PM, Adrian Crum wrote: > >> Here is a practical demonstration of the goal: >> ---------------------------------------------- >> >> I want to add a new attribute called "cool-attribute" to the form >> widget. The attribute will be used by the rendering classes only - the >> form widget does not perform any processing based on the new attribute. >> >> So, I add the new attribute to the xsd. Then I have to update the >> widget java classes. >> >> *** Using the existing code - >> >> public class ModelForm extends ModelWidget { >> ... >> protected String coolAttribute; >> ... >> this.coolAttribute = parent.coolAttribute; >> ... >> if (this.coolAttribute == null || >> formElement.hasAttribute("cool-attribute")) { >> this.coolAttribute = formElement.getAttribute("cool-attribute"); >> } >> ... >> public String getCoolAttribute() { >> return this.coolAttribute; >> } >> ... >> public void setCoolAttribute(String coolAttribute) { >> this.coolAttribute = coolAttribute; >> } >> } >> >> public class HtmlFormRenderer extends HtmlWidgetRenderer implements >> FormStringRenderer { >> ... >> String coolAttribute = modelForm.getCoolAttribute(); >> // do something with cool attribute >> } >> >> *** Using the proposed change - >> >> public class HtmlFormRenderer extends HtmlWidgetRenderer implements >> FormStringRenderer { >> ... >> String coolAttribute = modelForm.getAttribute("cool-attribute"); >> // do something with cool attribute >> } >> >> ---------------------------------------------- >> >> And there is my point - a very simple change is being made >> unnecessarily complex. > > This sort of thing is exactly why for business logic we use generic > structures like Maps (and GenericValue objects...) instead of custom > objects for each little thing. > > For the framework though, even though it is more verbose those things DO > have value and make things perform better (a variable access is faster > than a Map lookup, especially important for things we do over and over > which is mostly what the framework is), but even moreso it results in an > API that is far more "self-documenting" and that can be used with a good > variety of tools, etc. Also, in many cases getters and setters have a > benefit because you can add logic over time, or hide complexity. While > that is often not desired for business logic (which should be very > transparent) for frameworks and lower level code this can be a big > benefit and ultimately reduce the size and complexity of higher level > code, especially application code which is the highest priority to make > smaller (as opposed to the framework code). Ah. That's where we differ. I see the model widgets needing transparency so that the widget documentation resides in the widget's xsd, not in the model widget java api. I'll put the idea on the shelf for now. -Adrian |
|
|
I had done some playing around with alternate ways of XSD, Java Model
conversion and runtime transformation of model.. Here is an approach that seemed very clean to me FWIW: - Use XSD to specify API and documentation for XML forms. Specify as much as possible in XSD. - Use Jaxb to generate Java model for XSD. This could be cached so that XML Unmarshalling does not need to be done. Jaxb generates classes with inheritance structure that follows XSD. - Transform at runtime as needed. This could be done using proxies with flexible string expanders. Also I had a need to use enumeration and multiple variants of 'form'(had extended widget-form.xsd to have more than 'form' top level element) Inheritance and includes. This may be a way to have groups of attributes making it easy to see things together. <xs:attributeGroups> can include other attribute groups. Inheritance can be done in XSD as well. There was one other thing that i was not sure about.. From Model POV. ModelFormField has one FieldInfo. Each of the field types e.g. Text, Image, Hidden etc. are subclasses of FieldInfo. Wondering why it is that way. Would it have been better to have Text, Image, Hidden etc. as subclass of ModelFormField in Java and Similarily something like this from EmployeeForms.xml <form name="AddEmployeeSkills" type="single" target="createEmployeeSkill" default-map-name="partySkill"> <field name="partyId"><hidden/></field> <field name="skillTypeId" tooltip="${uiLabelMap.CommonRequired}" widget-style="required"> <drop-down> <entity-options description="${description}" entity-name="SkillType"> <entity-order-by field-name="description"/> </entity-options> </drop-down> </field> <field name="yearsExperience"><text/></field> <field name="rating"><text/></field> <field name="skillLevel"><text/></field> <field name="submitButton" title="${uiLabelMap.CommonCreate}" widget-style="smallSubmit"><submit button-type="button"/></field> </form> May be expressed as <form name="AddEmployeeSkills" type="single" target="createEmployeeSkill" default-map-name="partySkill"> <hidden-field name="partyId"></field> <drop-down-field name="skillTypeId" tooltip="${uiLabelMap.CommonRequired}" widget-style="required"> <entity-options description="${description}" entity-name="SkillType"> <entity-order-by field-name="description"/> </entity-options> </drop-down-field> <text-field name="yearsExperience"/> <text-field name="rating"/> <text-field name="skillLevel"/> <submit-field name="submitButton" title="${uiLabelMap.CommonCreate}" widget-style="smallSubmit" submit button-type="button"/> </form> It appears that given the one to one nature of ModelFormField and FieldInfo, it really is one object and FormFields are really specific types of fields. Inheritance in XSD can model this efficiently with Standard set of attributes in base class e.g. name, widget-style etc. I hope this is somewhat useful in terms of ideas. I realize it is way off from current. Harmeet |
«
Return to OFBiz - Dev
|
1 view|%1 views
| Free forum by Nabble | Edit this page |