--- 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. 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
