Thanks, I just studied this concept: http://www.adtmag.com/java/articleold.asp?id=1475, and I agree with it, but I decided to go ahead with the change here because this seems to be a different scenario.
-1 Remember one of the three basic OO principles: use virtual methods instead of switch according to a class marker.
This principle would be 100% applicable if, for example, within a Class Glen, I have a function getCapital(country) as follows:
class Glen { // bad
getCapital (Country country) {
if (country instanceof UnitedStates) return "Washington";
elsif (country instanceof Canada) return "Ottawa";
....
}
}Absolutely, 100%, it should be like this instead:
getCapital(Country country) { return country.getCapital();
}
But if the function is "haveGlenBuySomething(Country country)" this functionality, from an OO perspective, really should be maintained in the Glen class, not the various Country classes:
class Glen { // good
haveGlenBuySomething(Country country) {
if (country instanceof UnitedStates) buyCowboyHat();
elsif (country instanceof Canada) buyHockeySkates(); elsif (country instanceof Mexico) buySombrero(); ....
}
private buyCowboyHat() { ... }
private buyHockeySkates() { ... }
private buySombrero() { ... }
}*not*
country.buyWhatGlenWants(this); // bad
class Canada extends Country {
buyWhatGlenWants(Glen glen) { // bad
glen.buyHockeySkates();
}
}This is not a very good OO design in three ways (comparing buyWhatGlenWants(this) vs. getCapital()):
1.) The private functions buyXXXX in class Glen now have to be made public. (Likewise, in AbstractRenderer, all of the renderInlineXXXX need to move from protected to public to allow for such external calls from the InlineAreas.) (This doesn't occur with getCapital().)
2.) The Country classes get coupled with objects not relevant for them (here, class Glen), and hence become less pluggable. (Again, this doesn't occur with getCapital().) In our code, the InlineArea subclasses end up being unnecessarily tied to the renderers. 3.) The business logic for what Glen wants to buy while in Canada is being maintained in the wrong class. Should Glen choose to buy something else in Canada, class Canada needs updating. (On the contrary, for getCapital(), the country subclass is exactly the right object to update when its capital changes.)
and remove the bounce-back between Renderers and Area objects, further simplifying the coding.
But this is what keeps the renderers pluggable. If these methods are removed, every renderer must follow the same design.
I don't understand this point completely (using the country example above, everyone would need to buy hockey skates, but by moving the logic down to the person, class Joerg can buy something else.) But at any rate, the Area Tree-needing renderers all descend from AbstractRenderer whether or not this change is made, so they will still follow more or less the same design. But if there's a demand for this pluggability in the future, and this change will provide it, we can easily go back to it.
At any rate, I think the new code is cleaner and easier to follow now (and besides, we have plenty of instanceof's in AbstractRenderer elsewhere anyway!)
Thanks, Glen
