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
