You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ofbiz.apache.org by Adrian Crum <ad...@yahoo.com> on 2008/11/27 03:05:18 UTC
Discussion: Screen Widget Java Code Cleanup
I would like to work on cleaning up the screen widget Java code. Here are some of my ideas:
1. The screen widget model classes are trying to be too many things. As a result, they contain some messy and scary code. I'd like to see the model widget become more of a data structure, and move the rendering code into the rendering classes.
2. Use a true visitor pattern in the model widgets. The current pattern is what I would call "befuddled double dispatch with baggage." Methods like
public void renderWidgetString(Appendable writer, Map<String, Object> context, ScreenStringRenderer screenStringRenderer)
would become
public void accept(ScreenWidgetVisitor visitor).
Rendering classes would implement the ScreenWidgetVisitor interface. The model widgets won't need to be concerned with writers, contexts, or any other messy details.
3. Convert the existing artifact gathering code to a screen widget visitor. That would get the artifact gathering code out of the model widgets and put it where it belongs. It would probably simplify the artifact gathering code as well.
Once these steps are completed, the model widget classes will be simple immutable data structures. The only code most of the model classes will have is a constructor and the accept method.
The rendering code will be simplified, since there will be better separation of concerns.
I will probably start off with #1 - move the rendering code from the model classes to the rendering classes. Once the dust settles from that, I will introduce the visitor interfaces and visitor classes. The existing model methods will be deprecated. The model classes will be bloated while they support both patterns. When the time is right we can remove the deprecated methods and we'll have lean, well structured screen widget code.
What do you think?
-Adrian
Re: Discussion: Screen Widget Java Code Cleanup
Posted by Adrian Crum <ad...@yahoo.com>.
http://en.wikipedia.org/wiki/Visitor_pattern
--- On Wed, 11/26/08, BJ Freeman <bj...@free-man.net> wrote:
> From: BJ Freeman <bj...@free-man.net>
> Subject: Re: Discussion: Screen Widget Java Code Cleanup
> To: dev@ofbiz.apache.org
> Date: Wednesday, November 26, 2008, 6:50 PM
> where can I find out about the visitor model
>
> Adrian Crum sent the following on 11/26/2008 6:05 PM:
> > I would like to work on cleaning up the screen widget
> Java code. Here are some of my ideas:
> >
> > 1. The screen widget model classes are trying to be
> too many things. As a result, they contain some messy and
> scary code. I'd like to see the model widget become more
> of a data structure, and move the rendering code into the
> rendering classes.
> >
> > 2. Use a true visitor pattern in the model widgets.
> The current pattern is what I would call "befuddled
> double dispatch with baggage." Methods like
> >
> > public void renderWidgetString(Appendable writer,
> Map<String, Object> context, ScreenStringRenderer
> screenStringRenderer)
> >
> > would become
> >
> > public void accept(ScreenWidgetVisitor visitor).
> >
> > Rendering classes would implement the
> ScreenWidgetVisitor interface. The model widgets won't
> need to be concerned with writers, contexts, or any other
> messy details.
> >
> > 3. Convert the existing artifact gathering code to a
> screen widget visitor. That would get the artifact gathering
> code out of the model widgets and put it where it belongs.
> It would probably simplify the artifact gathering code as
> well.
> >
> > Once these steps are completed, the model widget
> classes will be simple immutable data structures. The only
> code most of the model classes will have is a constructor
> and the accept method.
> >
> > The rendering code will be simplified, since there
> will be better separation of concerns.
> >
> > I will probably start off with #1 - move the rendering
> code from the model classes to the rendering classes. Once
> the dust settles from that, I will introduce the visitor
> interfaces and visitor classes. The existing model methods
> will be deprecated. The model classes will be bloated while
> they support both patterns. When the time is right we can
> remove the deprecated methods and we'll have lean, well
> structured screen widget code.
> >
> > What do you think?
> >
> > -Adrian
> >
> >
> >
> >
> >
> >
Re: Discussion: Screen Widget Java Code Cleanup
Posted by BJ Freeman <bj...@free-man.net>.
where can I find out about the visitor model
Adrian Crum sent the following on 11/26/2008 6:05 PM:
> I would like to work on cleaning up the screen widget Java code. Here are some of my ideas:
>
> 1. The screen widget model classes are trying to be too many things. As a result, they contain some messy and scary code. I'd like to see the model widget become more of a data structure, and move the rendering code into the rendering classes.
>
> 2. Use a true visitor pattern in the model widgets. The current pattern is what I would call "befuddled double dispatch with baggage." Methods like
>
> public void renderWidgetString(Appendable writer, Map<String, Object> context, ScreenStringRenderer screenStringRenderer)
>
> would become
>
> public void accept(ScreenWidgetVisitor visitor).
>
> Rendering classes would implement the ScreenWidgetVisitor interface. The model widgets won't need to be concerned with writers, contexts, or any other messy details.
>
> 3. Convert the existing artifact gathering code to a screen widget visitor. That would get the artifact gathering code out of the model widgets and put it where it belongs. It would probably simplify the artifact gathering code as well.
>
> Once these steps are completed, the model widget classes will be simple immutable data structures. The only code most of the model classes will have is a constructor and the accept method.
>
> The rendering code will be simplified, since there will be better separation of concerns.
>
> I will probably start off with #1 - move the rendering code from the model classes to the rendering classes. Once the dust settles from that, I will introduce the visitor interfaces and visitor classes. The existing model methods will be deprecated. The model classes will be bloated while they support both patterns. When the time is right we can remove the deprecated methods and we'll have lean, well structured screen widget code.
>
> What do you think?
>
> -Adrian
>
>
>
>
>
>
Re: Discussion: Screen Widget Java Code Cleanup
Posted by Adrian Crum <ad...@yahoo.com>.
Harmeet,
That is exactly what I was trying to describe. The idea behind screen widgets is to have them "rendering format agnostic" - and the visitor pattern is perfect for that.
-Adrian
--- On Wed, 11/26/08, Harmeet Bedi <ha...@gmail.com> wrote:
> From: Harmeet Bedi <ha...@gmail.com>
> Subject: Re: Discussion: Screen Widget Java Code Cleanup
> To: dev@ofbiz.apache.org
> Date: Wednesday, November 26, 2008, 11:51 PM
> My understand is that the value of visitor is: have simple
> data structures and code/attach what is done on the data
> structure using visitors.
>
> This may seem odd given that one should have operations
> close to data structure but it turns out very useful esp. if
> there are classes of operations done on number of data
> structures. Each class of operations becomes one visitor,
> making operation addition easier.
>
> So as an example - take 2 classes of operations - render to
> html to render to pdf on same set of data structures X, Y,
> Z.
> In visitor world one would write two visitors and make X,Y,
> Z visitable(i.e. have accept method)
> If methods were close to datastructures, one would write 2
> types of rendering within class representing X,Y,Z
>
> Having one visitor per rendering has the benefit that all
> the rendering code is in one visitor implementaion. This
> makes it easy to refactor/reuse. I have seen code get
> cleaned up a lot when using visitors.
>
> I am not sure how this would apply to ofbiz code.. but it
> seems that proposal intent is to take things like
> Appendable writer, ScreenStringRenderer
> screenStringRenderer out of widget/screen code.
> Visitor will have these and access to datastructure (on
> acceptance of visitor by visitable class e.g.
> ModelScreenWidget) and will do right string appending.
>
> hope this was useful.
>
> harmeet
Re: Discussion: Screen Widget Java Code Cleanup
Posted by Harmeet Bedi <ha...@gmail.com>.
My understand is that the value of visitor is: have simple data
structures and code/attach what is done on the data structure using
visitors.
This may seem odd given that one should have operations close to data
structure but it turns out very useful esp. if there are classes of
operations done on number of data structures. Each class of operations
becomes one visitor, making operation addition easier.
So as an example - take 2 classes of operations - render to html to
render to pdf on same set of data structures X, Y, Z.
In visitor world one would write two visitors and make X,Y, Z
visitable(i.e. have accept method)
If methods were close to datastructures, one would write 2 types of
rendering within class representing X,Y,Z
Having one visitor per rendering has the benefit that all the rendering
code is in one visitor implementaion. This makes it easy to
refactor/reuse. I have seen code get cleaned up a lot when using visitors.
I am not sure how this would apply to ofbiz code.. but it seems that
proposal intent is to take things like
Appendable writer, ScreenStringRenderer screenStringRenderer out of
widget/screen code.
Visitor will have these and access to datastructure (on acceptance of
visitor by visitable class e.g. ModelScreenWidget) and will do right
string appending.
hope this was useful.
harmeet
Re: Discussion: Screen Widget Java Code Cleanup
Posted by Bilgin Ibryam <bi...@iguanait.com>.
Adrian Crum-2 wrote:
>
> --- On Wed, 11/26/08, David E Jones <da...@hotwaxmedia.com> wrote:
>> Do you have any specific bits of messy code in mind? I
>> don't mean to imply that I don't believe this might
>> help, but how does moving the code from one class to another
>> clean up the code?
>
> I didn't want to go into specifics because I didn't want to hurt any
> feelings or step on any toes. But here goes - using the current
> ModelScreenWidget.java as an example:
>
> 1. Line 797, 899, 1236 - HTML specific code inserted into model widget.
> 2. Lines 972 to 986 - lost indirection.
> 3. Line 1072 - non thread-safe code. Btw, after I make my proposed
> changes, code like that won't even compile.
> 4. Any method whose name starts with "set" - these classes are supposed to
> be immutable.
> 5. Any method that pushes and pops the context MapStack - that's a
> rendering detail the model widget shouldn't be concerned with.
> 6. Any method that makes multiple calls to the ScreenStringRenderer
> interface - the model class is trying to force a rendering sequence. This
> was a problem for me a couple of years ago when I tried to do some work on
> the tree widget.
>
>
I think using some design patterns in widget code may help committers to
decide what should go in the project and what not. Also may help to people
who are familiar with this patters, easily understand the widget code.
Just my 2 cents.
Bilgin
PS: In OFBIZ-1935, I added methods to ModelWidget which use globalContext.
Do you think that this breaks any design pattern used already in widgets?
--
View this message in context: http://www.nabble.com/Discussion%3A-Screen-Widget-Java-Code-Cleanup-tp20712463p20912034.html
Sent from the OFBiz - Dev mailing list archive at Nabble.com.
Re: Reverse bounty idea
Posted by Jacques Le Roux <ja...@les7arts.com>.
And GH did better https://docs.github.com/en/github/supporting-the-open-source-community-with-github-sponsors/about-github-sponsors
Jacques
Le 10/12/2013 à 11:47, Jacques Le Roux a écrit :
> Someone did it http://www.indiegogo.com/projects/a-month-of-apache-tapestry-5
>
> Jacques
>
> On Saturday, November 29, 2008 9:16 AM Jacques Le Roux <ja...@les7arts.com> wrote:
>> While working on https://issues.apache.org/jira/browse/OFBIZ-2058 I had also an idea about how to deal with OFBiz requirements
>> from a community POV.
>>
>> In Open source world there is the well known notion of bounty for feature.
>>> From time to time (rarely) someone offers to pay for a feature he/she needs.
>> But this is not used in an official way in OFBiz yet (there are no arranged place for that maybe because there are too few needs).
>>
>> Actually the idea I had is a reversed bounty concept. It will not be a person or organisation that will make a pledge for a
>> feature but a person or organisation that will submit a quote for a new feature to be commited in OFBiz.
>>
>> This came to my mind while working on a quote for a VAT stuff.
>> Let me explain : a lot of people need VAT/GST (I guess most countries but USA). I have open a Jira issue to centralise efforts
>> OFBIZ-366.
>> David Garett (OFBIZ-416) and Marco Risaliti (OFBIZ-1262) contributed big patches in the past and I wanted to review them but did
>> not find the time to do so seriously and was not sure it was the right way to go at this time.
>> Also one thing that discouraged me was that both David and Marco was seeing their patches as not ready to be commited. They are
>> now partially unmergeable but I'm sure with still some interesting ideas/code in them.
>> There is also an interesting recent thread http://markmail.org/message/bkqmwwvswzlgzbfr and an even more recent wiki page
>> http://docs.ofbiz.org/display/OFBIZ/VAT (actually these informations are referenced at OFBIZ-366 bottom).
>>
>> The reversed bounty concept is applicable for "feature" like VAT which actually may cover a lot of things (which are not obvious
>> at 1st sight).
>>
>> How I see it : one person, a team or an organisation propose to develop a new feature that has proven in the past to interest a
>> lot of people but has not been developed in OFBiz because being an huge work or not well enough defined
>> Actually by "propose" I mean detailled requirements to be freely discussed in dev ML. Could be also direct estimates like in
>> http://docs.ofbiz.org/display/OFBIZ/CRM+Sales+Force+Automation+Plan
>>
>> For instance, for VAT we know there have been some "proprietary" implementations.
>> I guess mostly because it seems difficult to coordinate efforts on things that at 1st sight appears to be countries or
>> organisations specific.
>>
>> But, from the links above, we know also that there are some commons features:
>> * periodic Tax Authority VAT report (input minus input) : by Tax Authority
>> * affect GL accounts (by Tax Authority) default being VAT Purchases/Sales respectively linked to Assets and Liabilities GL
>> accounts
>> * show prices VAT inclusive everywhere it's needed (some issues : OFBIZ-1086)
>> * how to apply VAT on discount (some categories, not so much)
>> * etc. (more details in VAT wiki page)
>>
>> Sorry for the long and disorganised post. I'm trying to find a new way to earn a living (crisis effect ?). Reverse bounties could
>> be shared of course...
>>
>> Jacques
>> PS : I was not the 1st to have this idea. Curious I just found that a Drupal developer has already suggested a such thing
Re: Reverse bounty idea
Posted by Jacques Le Roux <ja...@les7arts.com>.
Someone did it http://www.indiegogo.com/projects/a-month-of-apache-tapestry-5
Jacques
On Saturday, November 29, 2008 9:16 AM Jacques Le Roux <ja...@les7arts.com> wrote:
> While working on https://issues.apache.org/jira/browse/OFBIZ-2058 I had also an idea about how to deal with OFBiz requirements
> from a community POV.
>
> In Open source world there is the well known notion of bounty for feature.
>> From time to time (rarely) someone offers to pay for a feature he/she needs.
> But this is not used in an official way in OFBiz yet (there are no arranged place for that maybe because there are too few needs).
>
> Actually the idea I had is a reversed bounty concept. It will not be a person or organisation that will make a pledge for a
> feature but a person or organisation that will submit a quote for a new feature to be commited in OFBiz.
>
> This came to my mind while working on a quote for a VAT stuff.
> Let me explain : a lot of people need VAT/GST (I guess most countries but USA). I have open a Jira issue to centralise efforts
> OFBIZ-366.
> David Garett (OFBIZ-416) and Marco Risaliti (OFBIZ-1262) contributed big patches in the past and I wanted to review them but did
> not find the time to do so seriously and was not sure it was the right way to go at this time.
> Also one thing that discouraged me was that both David and Marco was seeing their patches as not ready to be commited. They are
> now partially unmergeable but I'm sure with still some interesting ideas/code in them.
> There is also an interesting recent thread http://markmail.org/message/bkqmwwvswzlgzbfr and an even more recent wiki page
> http://docs.ofbiz.org/display/OFBIZ/VAT (actually these informations are referenced at OFBIZ-366 bottom).
>
> The reversed bounty concept is applicable for "feature" like VAT which actually may cover a lot of things (which are not obvious
> at 1st sight).
>
> How I see it : one person, a team or an organisation propose to develop a new feature that has proven in the past to interest a
> lot of people but has not been developed in OFBiz because being an huge work or not well enough defined
> Actually by "propose" I mean detailled requirements to be freely discussed in dev ML. Could be also direct estimates like in
> http://docs.ofbiz.org/display/OFBIZ/CRM+Sales+Force+Automation+Plan
>
> For instance, for VAT we know there have been some "proprietary" implementations.
> I guess mostly because it seems difficult to coordinate efforts on things that at 1st sight appears to be countries or
> organisations specific.
>
> But, from the links above, we know also that there are some commons features:
> * periodic Tax Authority VAT report (input minus input) : by Tax Authority
> * affect GL accounts (by Tax Authority) default being VAT Purchases/Sales respectively linked to Assets and Liabilities GL
> accounts
> * show prices VAT inclusive everywhere it's needed (some issues : OFBIZ-1086)
> * how to apply VAT on discount (some categories, not so much)
> * etc. (more details in VAT wiki page)
>
> Sorry for the long and disorganised post. I'm trying to find a new way to earn a living (crisis effect ?). Reverse bounties could
> be shared of course...
>
> Jacques
> PS : I was not the 1st to have this idea. Curious I just found that a Drupal developer has already suggested a such thing
Reverse bounty idea
Posted by Jacques Le Roux <ja...@les7arts.com>.
While working on https://issues.apache.org/jira/browse/OFBIZ-2058 I had also an idea about how to deal with OFBiz requirements from
a community POV.
In Open source world there is the well known notion of bounty for feature.
>From time to time (rarely) someone offers to pay for a feature he/she needs.
But this is not used in an official way in OFBiz yet (there are no arranged place for that maybe because there are too few needs).
Actually the idea I had is a reversed bounty concept. It will not be a person or organisation that will make a pledge for a feature
but a person or organisation that will submit a quote for a new feature to be commited in OFBiz.
This came to my mind while working on a quote for a VAT stuff.
Let me explain : a lot of people need VAT/GST (I guess most countries but USA). I have open a Jira issue to centralise efforts
OFBIZ-366.
David Garett (OFBIZ-416) and Marco Risaliti (OFBIZ-1262) contributed big patches in the past and I wanted to review them but did not
find the time to do so seriously and was not sure it was the right way to go at this time.
Also one thing that discouraged me was that both David and Marco was seeing their patches as not ready to be commited. They are now
partially unmergeable but I'm sure with still some interesting ideas/code in them.
There is also an interesting recent thread http://markmail.org/message/bkqmwwvswzlgzbfr and an even more recent wiki page
http://docs.ofbiz.org/display/OFBIZ/VAT (actually these informations are referenced at OFBIZ-366 bottom).
The reversed bounty concept is applicable for "feature" like VAT which actually may cover a lot of things (which are not obvious at
1st sight).
How I see it : one person, a team or an organisation propose to develop a new feature that has proven in the past to interest a lot
of people but has not been developed in OFBiz because being an huge work or not well enough defined
Actually by "propose" I mean detailled requirements to be freely discussed in dev ML. Could be also direct estimates like in
http://docs.ofbiz.org/display/OFBIZ/CRM+Sales+Force+Automation+Plan
For instance, for VAT we know there have been some "proprietary" implementations.
I guess mostly because it seems difficult to coordinate efforts on things that at 1st sight appears to be countries or
organisations specific.
But, from the links above, we know also that there are some commons features:
* periodic Tax Authority VAT report (input minus input) : by Tax Authority
* affect GL accounts (by Tax Authority) default being VAT Purchases/Sales respectively linked to Assets and Liabilities GL accounts
* show prices VAT inclusive everywhere it's needed (some issues : OFBIZ-1086)
* how to apply VAT on discount (some categories, not so much)
* etc. (more details in VAT wiki page)
Sorry for the long and disorganised post. I'm trying to find a new way to earn a living (crisis effect ?). Reverse bounties could be
shared of course...
Jacques
PS : I was not the 1st to have this idea. Curious I just found that a Drupal developer has already suggested a such thing
Re: Discussion: Screen Widget Java Code Cleanup
Posted by Jacques Le Roux <ja...@les7arts.com>.
From: "David E Jones" <da...@hotwaxmedia.com>
>
> On Nov 27, 2008, at 10:52 AM, Adrian Crum wrote:
>
>>>> 3. Convert the existing artifact gathering code to a
>>> screen widget visitor. That would get the artifact gathering
>>> code out of the model widgets and put it where it belongs.
>>> It would probably simplify the artifact gathering code as
>>> well.
>>>
>>> I'm not sure what this means... but I might be missing
>>> something obvious. Hopefully someone else has more helpful
>>> comments.
>>
>> I can't think of a simpler way to explain it. The artifact info
>> gathering code belongs in the webtools component, not in the model
>> widget classes. In other words, it's okay for artifact info
>> gathering classes to know about model widget classes, but model
>> widget classes shouldn't have to know about artifact info gathering.
>
> I see, the Artifact Info stuff.
>
> Actually, I'd prefer the opposite. IMO things should be as self-
> describing and know as much about themselves and things they depend on
> as possible. That's something I'd really rather not externalize. In
> fact, it would have been really cool if this stuff just used for
> artifact info was inherent in the classes, but because things are
> looked at differently than how they are when they are actually used it
> doesn't quite work that way, or more importantly sometimes it DOES
> work that way so it would be a pain to have that separated.
>
> Either way, this stuff certainly doesn't belong in the webtools
> webapp. The actual intended use of this code was originally to be data
> gathering code for an IDE plugin. So far no one has picked that
> project up, and we needed something to test this stuff with (and it is
> moderately useful too), so that is why there is a very basic UI for it
> in webtools.
>
> -David
I saw BJ speaking about artifact info many times before I really used it this week.
I understand now why he was so interested. For instance, one hard thing to deal with in OFBiz is to create a mental path from, say,
an event to a service called thru an ECA, called by and ECA, call....
I experienced that this week while working on https://issues.apache.org/jira/browse/OFBIZ-2058.
I then thought about how sweet would be an UI for that (Eclipse plugin for instance...).
Jacques
Re: Discussion: Screen Widget Java Code Cleanup
Posted by Jacques Le Roux <ja...@les7arts.com>.
Interesting discussion BTW. "By luck" I never used the Visitor pattern before but have now a good idea of when to use it or not :o)
Thanks
Jacques
From: "Adrian Crum" <ad...@yahoo.com>
> Okay, we have a design philosophy difference. I'll put this back on the shelf and come back to it again later.
>
> -Adrian
>
>
> --- On Thu, 11/27/08, David E Jones <da...@hotwaxmedia.com> wrote:
>
>> From: David E Jones <da...@hotwaxmedia.com>
>> Subject: Re: Discussion: Screen Widget Java Code Cleanup
>> To: dev@ofbiz.apache.org
>> Date: Thursday, November 27, 2008, 12:54 PM
>> On Nov 27, 2008, at 10:52 AM, Adrian Crum wrote:
>>
>> > --- On Wed, 11/26/08, David E Jones
>> <da...@hotwaxmedia.com> wrote:
>> >> Do you have any specific bits of messy code in
>> mind? I
>> >> don't mean to imply that I don't believe
>> this might
>> >> help, but how does moving the code from one class
>> to another
>> >> clean up the code?
>> >
>> > I didn't want to go into specifics because I
>> didn't want to hurt any feelings or step on any toes.
>>
>> People choose to hurt their own feelings. Being sensitive
>> and not willing to discuss things just hurts us all... so
>> off we go...
>>
>> > But here goes - using the current
>> ModelScreenWidget.java as an example:
>> >
>> > 1. Line 797, 899, 1236 - HTML specific code inserted
>> into model widget.
>> > 2. Lines 972 to 986 - lost indirection.
>> > 3. Line 1072 - non thread-safe code. Btw, after I make
>> my proposed changes, code like that won't even compile.
>>
>> These sound like good things to address directly, and that
>> wouldn't be fixed by following the visitor pattern.
>> Separating the render control code from the modeling code
>> might be helpful (the "rendering" code is already
>> separate... or should be... I haven't watched every
>> patch going in here as much as I should... which means
>> sometimes things are committed by certain people who
>> don't review them, so the code was NEVER reviewed).
>>
>> > 4. Any method whose name starts with "set" -
>> these classes are supposed to be immutable.
>>
>> Are they? There is a risk of changing things in a way that
>> is not thread-safe, but I don't think they were ever
>> meant to be, or designed to be, unchangeable.
>>
>> > 5. Any method that pushes and pops the context
>> MapStack - that's a rendering detail the model widget
>> shouldn't be concerned with.
>>
>> If you mean to separate rendering from modeling, but this
>> is more "rendering control" than actual rendering.
>> I certainly wouldn't want the platform-specific
>> rendering code to be handling this.
>>
>> > 6. Any method that makes multiple calls to the
>> ScreenStringRenderer interface - the model class is trying
>> to force a rendering sequence. This was a problem for me a
>> couple of years ago when I tried to do some work on the tree
>> widget.
>>
>> I don't think we can go after complete flexibility for
>> rendering sequence. Specific things may need to be
>> refactored over time as rendering for new
>> "platforms" is added, but while this is hard to
>> predict we can't just have no rendering order
>> whatsoever.
>>
>> > This is just one file out of a handful, and it's
>> the cleanest, least offending one.
>> >
>> >> Sorry if I'm a luddite, but this seems to make
>> the code
>> >> more difficult and makes me think things like:
>> what does
>> >> "accept" mean, and what would a method
>> named that
>> >> actually do? what is actually being passed into
>> this
>> >> method... maybe that can help me understand it...
>> oh...
>> >> what's a "visitor"?
>> >
>> > Luddites are free to ask questions on the mailing
>> list, just like non-luddites.
>> >
>> >
>> >> I guess what I'm saying is that just because
>> someone
>> >> writes up a practice and calls it a best practice
>> or a
>> >> "pattern" doesn't mean it's
>> always a good
>> >> idea.
>> >
>> > In this case, it is the most appropriate pattern.
>> It's the same pattern Adam used in the entity condition
>> code, and the pattern I used for the iCalendar integration.
>>
>> I wasn't referring to myself as a luddite because I
>> don't understand it. In fact, I'd like to think that
>> I understand it enough and in fact have enough experience
>> with it to know that it is used far more than it should be.
>> The entity condition code is a good example of this.
>>
>> The visitor pattern tends to make code difficult to follow
>> and understand, and therefore to debug and maintain. This is
>> usually true even for the person who wrote the original
>> code. While I wrote the original entity condition code Adam
>> Heath is the one who refactored it to incorporate various
>> uses of the visitor pattern. After he did that it took me
>> WAY longer to work through issues in it and expand it. Now a
>> few years later Adam is getting back into that code and his
>> recent comment to me was that even he was having a hard time
>> following it now.
>>
>> I should have seen this before, but at the time I
>> didn't have experience with it. Now my opinion is that
>> the visitor pattern obfuscates and complicates code, and
>> usually the problems that people _think_ will somehow be
>> solved "inherently" by using it are not solved at
>> all, and in fact are aggravated by using it because now
>> things are more difficult to track down and follow. Thinking
>> back on it, I should have seen that before. The visitor
>> pattern is a bad thing. That's the position I'll
>> assume based on study and experience, thank you very much.
>>
>> That is the reason for my comments on the alternate code
>> you discussed. Passing in the renderer interface
>> implementation allows us to keep the platform-specific
>> rendering code in a separate class, and does so in a very
>> literal way. I'd rather not hide all of that behind a
>> visitor object.
>>
>> The ONLY thing that I can see that the visitor pattern is
>> good for is to eliminate the need to change method
>> signatures as things are changed over time. That is why it
>> (or something like it, not really such a literal
>> interpretation of it) has been discussed for the
>> ShoppingCart, especially the constructors for the cart item
>> which tend to change over time, and are ridiculously large
>> right now (more than 5-6 parameters gets hard to keep track
>> of when you're trying to remember which is which, and
>> there is nothing like attribute names in XML to help you
>> out).
>>
>> >>> 3. Convert the existing artifact gathering
>> code to a
>> >> screen widget visitor. That would get the artifact
>> gathering
>> >> code out of the model widgets and put it where it
>> belongs.
>> >> It would probably simplify the artifact gathering
>> code as
>> >> well.
>> >>
>> >> I'm not sure what this means... but I might be
>> missing
>> >> something obvious. Hopefully someone else has more
>> helpful
>> >> comments.
>> >
>> > I can't think of a simpler way to explain it. The
>> artifact info gathering code belongs in the webtools
>> component, not in the model widget classes. In other words,
>> it's okay for artifact info gathering classes to know
>> about model widget classes, but model widget classes
>> shouldn't have to know about artifact info gathering.
>>
>> I see, the Artifact Info stuff.
>>
>> Actually, I'd prefer the opposite. IMO things should be
>> as self-describing and know as much about themselves and
>> things they depend on as possible. That's something
>> I'd really rather not externalize. In fact, it would
>> have been really cool if this stuff just used for artifact
>> info was inherent in the classes, but because things are
>> looked at differently than how they are when they are
>> actually used it doesn't quite work that way, or more
>> importantly sometimes it DOES work that way so it would be a
>> pain to have that separated.
>>
>> Either way, this stuff certainly doesn't belong in the
>> webtools webapp. The actual intended use of this code was
>> originally to be data gathering code for an IDE plugin. So
>> far no one has picked that project up, and we needed
>> something to test this stuff with (and it is moderately
>> useful too), so that is why there is a very basic UI for it
>> in webtools.
>>
>> -David
>
>
>
>
Re: Discussion: Screen Widget Java Code Cleanup
Posted by Adrian Crum <ad...@yahoo.com>.
Okay, we have a design philosophy difference. I'll put this back on the shelf and come back to it again later.
-Adrian
--- On Thu, 11/27/08, David E Jones <da...@hotwaxmedia.com> wrote:
> From: David E Jones <da...@hotwaxmedia.com>
> Subject: Re: Discussion: Screen Widget Java Code Cleanup
> To: dev@ofbiz.apache.org
> Date: Thursday, November 27, 2008, 12:54 PM
> On Nov 27, 2008, at 10:52 AM, Adrian Crum wrote:
>
> > --- On Wed, 11/26/08, David E Jones
> <da...@hotwaxmedia.com> wrote:
> >> Do you have any specific bits of messy code in
> mind? I
> >> don't mean to imply that I don't believe
> this might
> >> help, but how does moving the code from one class
> to another
> >> clean up the code?
> >
> > I didn't want to go into specifics because I
> didn't want to hurt any feelings or step on any toes.
>
> People choose to hurt their own feelings. Being sensitive
> and not willing to discuss things just hurts us all... so
> off we go...
>
> > But here goes - using the current
> ModelScreenWidget.java as an example:
> >
> > 1. Line 797, 899, 1236 - HTML specific code inserted
> into model widget.
> > 2. Lines 972 to 986 - lost indirection.
> > 3. Line 1072 - non thread-safe code. Btw, after I make
> my proposed changes, code like that won't even compile.
>
> These sound like good things to address directly, and that
> wouldn't be fixed by following the visitor pattern.
> Separating the render control code from the modeling code
> might be helpful (the "rendering" code is already
> separate... or should be... I haven't watched every
> patch going in here as much as I should... which means
> sometimes things are committed by certain people who
> don't review them, so the code was NEVER reviewed).
>
> > 4. Any method whose name starts with "set" -
> these classes are supposed to be immutable.
>
> Are they? There is a risk of changing things in a way that
> is not thread-safe, but I don't think they were ever
> meant to be, or designed to be, unchangeable.
>
> > 5. Any method that pushes and pops the context
> MapStack - that's a rendering detail the model widget
> shouldn't be concerned with.
>
> If you mean to separate rendering from modeling, but this
> is more "rendering control" than actual rendering.
> I certainly wouldn't want the platform-specific
> rendering code to be handling this.
>
> > 6. Any method that makes multiple calls to the
> ScreenStringRenderer interface - the model class is trying
> to force a rendering sequence. This was a problem for me a
> couple of years ago when I tried to do some work on the tree
> widget.
>
> I don't think we can go after complete flexibility for
> rendering sequence. Specific things may need to be
> refactored over time as rendering for new
> "platforms" is added, but while this is hard to
> predict we can't just have no rendering order
> whatsoever.
>
> > This is just one file out of a handful, and it's
> the cleanest, least offending one.
> >
> >> Sorry if I'm a luddite, but this seems to make
> the code
> >> more difficult and makes me think things like:
> what does
> >> "accept" mean, and what would a method
> named that
> >> actually do? what is actually being passed into
> this
> >> method... maybe that can help me understand it...
> oh...
> >> what's a "visitor"?
> >
> > Luddites are free to ask questions on the mailing
> list, just like non-luddites.
> >
> >
> >> I guess what I'm saying is that just because
> someone
> >> writes up a practice and calls it a best practice
> or a
> >> "pattern" doesn't mean it's
> always a good
> >> idea.
> >
> > In this case, it is the most appropriate pattern.
> It's the same pattern Adam used in the entity condition
> code, and the pattern I used for the iCalendar integration.
>
> I wasn't referring to myself as a luddite because I
> don't understand it. In fact, I'd like to think that
> I understand it enough and in fact have enough experience
> with it to know that it is used far more than it should be.
> The entity condition code is a good example of this.
>
> The visitor pattern tends to make code difficult to follow
> and understand, and therefore to debug and maintain. This is
> usually true even for the person who wrote the original
> code. While I wrote the original entity condition code Adam
> Heath is the one who refactored it to incorporate various
> uses of the visitor pattern. After he did that it took me
> WAY longer to work through issues in it and expand it. Now a
> few years later Adam is getting back into that code and his
> recent comment to me was that even he was having a hard time
> following it now.
>
> I should have seen this before, but at the time I
> didn't have experience with it. Now my opinion is that
> the visitor pattern obfuscates and complicates code, and
> usually the problems that people _think_ will somehow be
> solved "inherently" by using it are not solved at
> all, and in fact are aggravated by using it because now
> things are more difficult to track down and follow. Thinking
> back on it, I should have seen that before. The visitor
> pattern is a bad thing. That's the position I'll
> assume based on study and experience, thank you very much.
>
> That is the reason for my comments on the alternate code
> you discussed. Passing in the renderer interface
> implementation allows us to keep the platform-specific
> rendering code in a separate class, and does so in a very
> literal way. I'd rather not hide all of that behind a
> visitor object.
>
> The ONLY thing that I can see that the visitor pattern is
> good for is to eliminate the need to change method
> signatures as things are changed over time. That is why it
> (or something like it, not really such a literal
> interpretation of it) has been discussed for the
> ShoppingCart, especially the constructors for the cart item
> which tend to change over time, and are ridiculously large
> right now (more than 5-6 parameters gets hard to keep track
> of when you're trying to remember which is which, and
> there is nothing like attribute names in XML to help you
> out).
>
> >>> 3. Convert the existing artifact gathering
> code to a
> >> screen widget visitor. That would get the artifact
> gathering
> >> code out of the model widgets and put it where it
> belongs.
> >> It would probably simplify the artifact gathering
> code as
> >> well.
> >>
> >> I'm not sure what this means... but I might be
> missing
> >> something obvious. Hopefully someone else has more
> helpful
> >> comments.
> >
> > I can't think of a simpler way to explain it. The
> artifact info gathering code belongs in the webtools
> component, not in the model widget classes. In other words,
> it's okay for artifact info gathering classes to know
> about model widget classes, but model widget classes
> shouldn't have to know about artifact info gathering.
>
> I see, the Artifact Info stuff.
>
> Actually, I'd prefer the opposite. IMO things should be
> as self-describing and know as much about themselves and
> things they depend on as possible. That's something
> I'd really rather not externalize. In fact, it would
> have been really cool if this stuff just used for artifact
> info was inherent in the classes, but because things are
> looked at differently than how they are when they are
> actually used it doesn't quite work that way, or more
> importantly sometimes it DOES work that way so it would be a
> pain to have that separated.
>
> Either way, this stuff certainly doesn't belong in the
> webtools webapp. The actual intended use of this code was
> originally to be data gathering code for an IDE plugin. So
> far no one has picked that project up, and we needed
> something to test this stuff with (and it is moderately
> useful too), so that is why there is a very basic UI for it
> in webtools.
>
> -David
Re: Discussion: Screen Widget Java Code Cleanup
Posted by David E Jones <da...@hotwaxmedia.com>.
On Nov 27, 2008, at 10:52 AM, Adrian Crum wrote:
> --- On Wed, 11/26/08, David E Jones <da...@hotwaxmedia.com>
> wrote:
>> Do you have any specific bits of messy code in mind? I
>> don't mean to imply that I don't believe this might
>> help, but how does moving the code from one class to another
>> clean up the code?
>
> I didn't want to go into specifics because I didn't want to hurt any
> feelings or step on any toes.
People choose to hurt their own feelings. Being sensitive and not
willing to discuss things just hurts us all... so off we go...
> But here goes - using the current ModelScreenWidget.java as an
> example:
>
> 1. Line 797, 899, 1236 - HTML specific code inserted into model
> widget.
> 2. Lines 972 to 986 - lost indirection.
> 3. Line 1072 - non thread-safe code. Btw, after I make my proposed
> changes, code like that won't even compile.
These sound like good things to address directly, and that wouldn't be
fixed by following the visitor pattern. Separating the render control
code from the modeling code might be helpful (the "rendering" code is
already separate... or should be... I haven't watched every patch
going in here as much as I should... which means sometimes things are
committed by certain people who don't review them, so the code was
NEVER reviewed).
> 4. Any method whose name starts with "set" - these classes are
> supposed to be immutable.
Are they? There is a risk of changing things in a way that is not
thread-safe, but I don't think they were ever meant to be, or designed
to be, unchangeable.
> 5. Any method that pushes and pops the context MapStack - that's a
> rendering detail the model widget shouldn't be concerned with.
If you mean to separate rendering from modeling, but this is more
"rendering control" than actual rendering. I certainly wouldn't want
the platform-specific rendering code to be handling this.
> 6. Any method that makes multiple calls to the ScreenStringRenderer
> interface - the model class is trying to force a rendering sequence.
> This was a problem for me a couple of years ago when I tried to do
> some work on the tree widget.
I don't think we can go after complete flexibility for rendering
sequence. Specific things may need to be refactored over time as
rendering for new "platforms" is added, but while this is hard to
predict we can't just have no rendering order whatsoever.
> This is just one file out of a handful, and it's the cleanest, least
> offending one.
>
>> Sorry if I'm a luddite, but this seems to make the code
>> more difficult and makes me think things like: what does
>> "accept" mean, and what would a method named that
>> actually do? what is actually being passed into this
>> method... maybe that can help me understand it... oh...
>> what's a "visitor"?
>
> Luddites are free to ask questions on the mailing list, just like
> non-luddites.
>
>
>> I guess what I'm saying is that just because someone
>> writes up a practice and calls it a best practice or a
>> "pattern" doesn't mean it's always a good
>> idea.
>
> In this case, it is the most appropriate pattern. It's the same
> pattern Adam used in the entity condition code, and the pattern I
> used for the iCalendar integration.
I wasn't referring to myself as a luddite because I don't understand
it. In fact, I'd like to think that I understand it enough and in fact
have enough experience with it to know that it is used far more than
it should be. The entity condition code is a good example of this.
The visitor pattern tends to make code difficult to follow and
understand, and therefore to debug and maintain. This is usually true
even for the person who wrote the original code. While I wrote the
original entity condition code Adam Heath is the one who refactored it
to incorporate various uses of the visitor pattern. After he did that
it took me WAY longer to work through issues in it and expand it. Now
a few years later Adam is getting back into that code and his recent
comment to me was that even he was having a hard time following it now.
I should have seen this before, but at the time I didn't have
experience with it. Now my opinion is that the visitor pattern
obfuscates and complicates code, and usually the problems that people
_think_ will somehow be solved "inherently" by using it are not solved
at all, and in fact are aggravated by using it because now things are
more difficult to track down and follow. Thinking back on it, I should
have seen that before. The visitor pattern is a bad thing. That's the
position I'll assume based on study and experience, thank you very much.
That is the reason for my comments on the alternate code you
discussed. Passing in the renderer interface implementation allows us
to keep the platform-specific rendering code in a separate class, and
does so in a very literal way. I'd rather not hide all of that behind
a visitor object.
The ONLY thing that I can see that the visitor pattern is good for is
to eliminate the need to change method signatures as things are
changed over time. That is why it (or something like it, not really
such a literal interpretation of it) has been discussed for the
ShoppingCart, especially the constructors for the cart item which tend
to change over time, and are ridiculously large right now (more than
5-6 parameters gets hard to keep track of when you're trying to
remember which is which, and there is nothing like attribute names in
XML to help you out).
>>> 3. Convert the existing artifact gathering code to a
>> screen widget visitor. That would get the artifact gathering
>> code out of the model widgets and put it where it belongs.
>> It would probably simplify the artifact gathering code as
>> well.
>>
>> I'm not sure what this means... but I might be missing
>> something obvious. Hopefully someone else has more helpful
>> comments.
>
> I can't think of a simpler way to explain it. The artifact info
> gathering code belongs in the webtools component, not in the model
> widget classes. In other words, it's okay for artifact info
> gathering classes to know about model widget classes, but model
> widget classes shouldn't have to know about artifact info gathering.
I see, the Artifact Info stuff.
Actually, I'd prefer the opposite. IMO things should be as self-
describing and know as much about themselves and things they depend on
as possible. That's something I'd really rather not externalize. In
fact, it would have been really cool if this stuff just used for
artifact info was inherent in the classes, but because things are
looked at differently than how they are when they are actually used it
doesn't quite work that way, or more importantly sometimes it DOES
work that way so it would be a pain to have that separated.
Either way, this stuff certainly doesn't belong in the webtools
webapp. The actual intended use of this code was originally to be data
gathering code for an IDE plugin. So far no one has picked that
project up, and we needed something to test this stuff with (and it is
moderately useful too), so that is why there is a very basic UI for it
in webtools.
-David
Re: Discussion: Screen Widget Java Code Cleanup
Posted by Adrian Crum <ad...@yahoo.com>.
--- On Wed, 11/26/08, David E Jones <da...@hotwaxmedia.com> wrote:
> Do you have any specific bits of messy code in mind? I
> don't mean to imply that I don't believe this might
> help, but how does moving the code from one class to another
> clean up the code?
I didn't want to go into specifics because I didn't want to hurt any feelings or step on any toes. But here goes - using the current ModelScreenWidget.java as an example:
1. Line 797, 899, 1236 - HTML specific code inserted into model widget.
2. Lines 972 to 986 - lost indirection.
3. Line 1072 - non thread-safe code. Btw, after I make my proposed changes, code like that won't even compile.
4. Any method whose name starts with "set" - these classes are supposed to be immutable.
5. Any method that pushes and pops the context MapStack - that's a rendering detail the model widget shouldn't be concerned with.
6. Any method that makes multiple calls to the ScreenStringRenderer interface - the model class is trying to force a rendering sequence. This was a problem for me a couple of years ago when I tried to do some work on the tree widget.
This is just one file out of a handful, and it's the cleanest, least offending one.
> Sorry if I'm a luddite, but this seems to make the code
> more difficult and makes me think things like: what does
> "accept" mean, and what would a method named that
> actually do? what is actually being passed into this
> method... maybe that can help me understand it... oh...
> what's a "visitor"?
Luddites are free to ask questions on the mailing list, just like non-luddites.
> I guess what I'm saying is that just because someone
> writes up a practice and calls it a best practice or a
> "pattern" doesn't mean it's always a good
> idea.
In this case, it is the most appropriate pattern. It's the same pattern Adam used in the entity condition code, and the pattern I used for the iCalendar integration.
> > 3. Convert the existing artifact gathering code to a
> screen widget visitor. That would get the artifact gathering
> code out of the model widgets and put it where it belongs.
> It would probably simplify the artifact gathering code as
> well.
>
> I'm not sure what this means... but I might be missing
> something obvious. Hopefully someone else has more helpful
> comments.
I can't think of a simpler way to explain it. The artifact info gathering code belongs in the webtools component, not in the model widget classes. In other words, it's okay for artifact info gathering classes to know about model widget classes, but model widget classes shouldn't have to know about artifact info gathering.
-Adrian
Re: Discussion: Screen Widget Java Code Cleanup
Posted by David E Jones <da...@hotwaxmedia.com>.
On Nov 26, 2008, at 9:05 PM, Adrian Crum wrote:
> I would like to work on cleaning up the screen widget Java code.
> Here are some of my ideas:
>
> 1. The screen widget model classes are trying to be too many things.
> As a result, they contain some messy and scary code. I'd like to see
> the model widget become more of a data structure, and move the
> rendering code into the rendering classes.
Do you have any specific bits of messy code in mind? I don't mean to
imply that I don't believe this might help, but how does moving the
code from one class to another clean up the code?
> 2. Use a true visitor pattern in the model widgets. The current
> pattern is what I would call "befuddled double dispatch with
> baggage." Methods like
>
> public void renderWidgetString(Appendable writer, Map<String,
> Object> context, ScreenStringRenderer screenStringRenderer)
>
> would become
>
> public void accept(ScreenWidgetVisitor visitor).
>
> Rendering classes would implement the ScreenWidgetVisitor interface.
> The model widgets won't need to be concerned with writers, contexts,
> or any other messy details.
Sorry if I'm a luddite, but this seems to make the code more difficult
and makes me think things like: what does "accept" mean, and what
would a method named that actually do? what is actually being passed
into this method... maybe that can help me understand it... oh...
what's a "visitor"?
I guess what I'm saying is that just because someone writes up a
practice and calls it a best practice or a "pattern" doesn't mean it's
always a good idea.
> 3. Convert the existing artifact gathering code to a screen widget
> visitor. That would get the artifact gathering code out of the model
> widgets and put it where it belongs. It would probably simplify the
> artifact gathering code as well.
I'm not sure what this means... but I might be missing something
obvious. Hopefully someone else has more helpful comments.
-David