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 <[EMAIL PROTECTED]> wrote:
> From: David E Jones <[EMAIL PROTECTED]>
> 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
> <[EMAIL PROTECTED]> 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
