From: David E Jones <[EMAIL PROTECTED]>
Subject: Re: Discussion: Screen Widget Java Code Cleanup
To: [email protected]
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
