On Sep 20, 2005, at 17:40, Matthew L Daniel wrote:

Hi Matthew,

The least this would do is avoid a number of unnecessary calls to

You're on the right track, and maybe that "switch" impl would be an
80/20 win over the cost of the Right Way. But that huge if-instance case
is exactly what the Visitor pattern was created to solve.

Yep! My remark was completely focused on that isolated part in one of the +600 source files. It just seemed like the more elegant way to deal with it --since the FONodes all have that getNameId() method, it makes more sense to call that once and compare the resulting int value to the constants, rather than checking the class for one node possibly so many times. (Plus: using 'switch' offers the benefit of having fall-through cases, where you would need 'if-instanceof||instanceof||instanceof...') As a personal note: I'd even like to replace all string-comparisons of the form 'fo.getName().equals("fo:something")' in the FOTree with 'fo.getNameId() == FO_SOMETHING'. Not sure if that would save us much, but every little bit helps I guess. Even if both strings are internalized, 'int == int' outperforms 'String.equals(String)' by a significant factor (and that's still leaving small things like "fo:instream-foreign-object" [=26 Bytes] vs. FO_INSTREAM_FOREIGN_OBJECT [=4 Bytes] out of the picture)

'instanceof' itself can IMO only be put to good use in case of a complex class-hierarchy, where you can check for supertypes that each have lots-and-lots of subtypes, so 'x instanceof supertype' covers ten or more subtypes in one go. Our FOTree isn't exactly suited for that kind of thing. Practically all the FOs are direct subclasses of FObj, and those that aren't, have at most one intermediate class in between... (I've taken the liberty of adding one especially for the table-related FOs, since they share some functionality --still have to take a closer look at which members can be moved upward to TableFObj. For now, it's restricted to a few methods for dealing with the column-numbering and the border-precedences, but I'm almost certain that more can be made to follow.)

I haven't spent enough time with the Fop codebase to speak to the gains of such a
thing, but I'd bet RTFHandler isn't the only one that does this.

Indeed not. FYI: FOP once did use the Visitor pattern for the layout-engine (over a year ago), but it was decided upon back then to remove it completely, mainly for reasons of keeping the code comprehensible for potential future committers IIRC.

A braindead `grep instanceof | wc -l` search shows 122 classes out of the
640 classes use instanceof, which in my world makes them at least worth

Depends a bit on *what* exactly they're checking and how many times they actually do that. If it's simple checks for FONode subtypes that are executed lots of times, these are indeed most likely suboptimal...

Another "implied" win to the Visitor interface is if a new FONode type
gets added, it forces the change to ripple (unless the impl is using the
DefaultVisitor) to all those who would need to handle the new

... which indeed becomes of very much use once you start talking about FO Extension-elements.

Thanks for the valuable feedback --and for being one of our guinea-pigs, of course ;-)



Reply via email to