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
instanceof.
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
examination.
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
functionality.
... 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 ;-)
Cheers,
Andreas
