Re: Visitor pattern vs if-ladder

On Fri, 24 Apr 2009, Giovanni Azua wrote:


That would be a remarkable thing to forget, given that that call is the
only thing accept has to do. This is accept:

public void accept(Visitor v) {
  v.visit(this);
}

So to forget to call visit, you'd have to write this:

public void accept(Visitor v) {
}

Which would be quite a surprising thing to write!

Unless you're thinking of the style of visitor which deals with composite
hierarchies, where elements deal with traversal of their children
(although your own code doesn't do this, so i assume this is not what you
mean), in which case the correct method looks like:

public void accept(Visitor v) {
  v.visit(this);
  // next bit could be more complicated
  for (Element child: getChildren()) {
  child.accept(v);
  }
}

In which case you might accidentally write:

public void accept(Visitor v) {
  // next bit could be more complicated
  for (Element child: getChildren()) {
  child.accept(v);
  }
}

Which would be more understandable, but is still a pretty unlikely error,
i'd say.


Write, yes, but it'd not something that takes much maintenance, is it?


Agreed - i don't think there's any way around the need to write accept in
each element class.


"Might cover a ghost requirement"! If you're adding visitor support to the
hierarchy, it's because you have a concrete requirement for it. I
certainly don't advocate going round adding visitor support to hierarchies
willy-nilly!


How so?


True. In a language which can do dispatch based on parameter types, you
only need one accept method (although i don't know how that could be
typesafe). But the fact is that java can't, so dismissing it as an
'artificial need' is sophistry.


True. If i couldn't change the hierarchy, i would have to consider an
alternative strategy, and i might well consider one like yours. But when i
can change the hierarchy, the classical visitor pattern is a better
solution.


Absolute nonsense! This "high maintenance toll" means adding methods to
deal with the new element types - which you *have to do* to maintain the
correctness of your program! Any way of avoiding this "toll" is simply a
way of writing an incorrect program! That is generally not considered a
good thing!


It's not simply the ugliness of the cow that bothers me, it's the fact
that the cow is being raised in unsanitary conditions, and is likely to
give its consumers CJD.


This is good. It's definitely useful to be able to do this to an existing
hierarchy.


Inadequate. Type safety, in this case, is not just about avoiding
ClassCastExceptions, it's about statically ensuring that all the input
types are dealt with properly. Your code does not do that, and it cannot
be made to do it.


It may not involve a reflective lookup, but it still involves a map
lookup. In fact, *three* map lookups, although it could easily be reduced
to one - where you currently do (eliding the details of creating a
visitor):

if (!theLookup.containsKey(myElementClassName)) {
  theLookup.put(myElementClassName, createVisitor());
}
if (theLookup.containsKey(myElementClassName)) {
  theLookup.get(myElementClassName).visit(anElement);
}

You could do:

IVisitor<E> visitor = theLookup.get(myElementClassName);
if (visitor == null) {
  visitor = createVisitor();
  if (visitor != null) theLookup.put(myElementClassName, visitor);
}
if (visitor != null) visitor.visit(anElement);

Even if you did that, a map lookup and a reflective invocation is still a
lot slower than two virtual invocations.

It's not just the performance which is shocking, though. There's also the
fact that findVisitorMethod is so unselective - it will pick any method
with the right parameter and return type as a visitor method, which means
that i have to be careful when writing my visitor class not to introduce
any methods which might be mistaken for visitor methods.

A couple of other queries. Firstly, why does VisitorDelegator override
getReturn with an implementation that simply does return
super.getReturn()? That's no different to not overriding it. Secondly, why
does isValidReturn do
aTestMethod.getReturnType().isAssignableFrom(aReturnClass), rather than
the other way round? If the specified return type is, say, String, then
that means that a method which returns Object will be classed as having a
valid return type. And that if the specified return type is Object, then
only methods declared as returning Object will be classed as valid, and
not any subtypes of Object!


Glad to hear it.


I don't claim to be able to produce a ClassCastException, but a test case
that breaks your code is easy. Write a new class:

public class Gearbox implements ICarPart {}

Modify Car to add:

private final Gearbox theGearbox = new Gearbox();
public Gearbox getGearbox() {
  return theGearbox;
}

Modify PrintVisitor.visitCar to add, at the end:

visit(aCar.getGearbox());

This will compile and run without error. However, there is no visitor
method which takes a Gearbox, so it won't be visited, and that's a bug.


Wow, an argument from authority and an ad hominem attack in the same
sentence. Impressive. Now see if you can get three fallacies in one
sentence!

Bertrand Meyer is a terribly famous guy, but it doesn't change the fact
that he's quite wrong in this case. Amusingly, he tries to spin the safety
hole that i've pointed out as an advantage - if you're not interested in
visiting a given type, you just don't write a method for it! A fine
example of why academics shouldn't try to give advice on the production of
software.

Nonetheless, if you do have a situation where you want to ignore all types
of element except a select few, that's easily dealt with with the
classical visitor. You just define a class NullVisitor or IgnoringVisitor,
which implements the visitor interface with no-op methods. Visitor classes
which want to be selective extend this, and override the methods for the
types they're interested in. Any other types, including any types added
after the visitor was written, will be ignored.

tom

--
.... the gripping first chapter, which literally grips you because it's
printed on a large clamp.