Visitor

Hello /prog/,
I'd like to know what do you think of this. When you must add an operation that only affects some elements of a list based on their type, would you use a visitor, or instanceof?

Here's an example where I want to power all the engines of a space ship. Would you make different lists for every type of parts, use instanceof ShipEngine, or use a visitor?

import java.util.LinkedList;
import java.util.List;

public class SpaceShip
{

// The ship parts

public static interface ShipPart
{
void accept(ShipPartVisitor visitor);
}

public static class ShipEngine implements ShipPart
{
public int power = 0;

@Override
public void accept(ShipPartVisitor visitor)
{
visitor.visit(this);
}

}

public static class ShipWeapon implements ShipPart
{
public int damage = 5, cooldown = 2;

@Override
public void accept(ShipPartVisitor visitor)
{
visitor.visit(this);
}
}

// The interface for visitors

public static interface ShipPartVisitor
{
void visit(ShipEngine shipEngine);

void visit(ShipWeapon shipWeapon);
}

// The methods and fields of the SpaceShip class

private final List<ShipPart> parts = new LinkedList<>();

public void addPart(ShipPart part)
{
parts.add(part);
}

public void setEnginesPower(final int powerLevel)
{
ShipPartVisitor visitor = new ShipPartVisitor()
{

@Override
public void visit(ShipWeapon shipWeapon)
{
// we're not playing with the weapons
}

@Override
public void visit(ShipEngine shipEngine)
{
shipEngine.power = powerLevel;
System.err.println("POWER LEVEL : " + powerLevel);
}
};

for (ShipPart pickedPart : parts)
pickedPart.accept(visitor);
}

public static void main(String[] args)
{
SpaceShip myShip = new SpaceShip();

// two engines and a weapon

myShip.addPart(new ShipEngine());
myShip.addPart(new ShipEngine());
myShip.addPart(new ShipWeapon());

// power on the engines!

myShip.setEnginesPower(100);
}

}

When I need to write something that only affects things of one ``type'', I make it affect things of that type, goddammit. setEnginesPower only sets the power of engines, so why do you go through all the boilerplate of creating an object that could theoretically do something to things that are not engines, only to explicitly disable that non-engine-affecting code?

Here. Try this. I left out all the stuff like error checking and used funny memory handling to save space, but the whole thing is bullshit anyway. Instead of writing some visit/accept shit, just write a goddamn function and call it.


/* gcc -o ship -std=c99 -pedantic -Wall ship.c */

#include <stdio.h>
#include <stdlib.h>

struct engine { char *id; int power; };
struct weapon { char *id; int dakka; };
struct ship { char *id; struct engine *engines; struct weapon *weapons; };

static void set_engines_power(struct ship *s, int p)
{
struct engine *e = s->engines;
printf("Setting engines of %s to power %d\n", s->id, p);
while (e->id) {
e->power = p;
++e;
}
}

static void describe_ship(struct ship *s)
{
struct engine *e = s->engines;
struct weapon *w = s->weapons;
printf("Ship '%s':\n", s->id);
while (e->id) {printf("\tEngine '%s' has %d power.\n", e->id, e->power); e++;}
while (w->id) {printf("\tWeapon '%s' has %d dakka.\n", w->id, w->dakka); w++;}
}

/* ... */

int main(void)
{
struct ship s = (struct ship){"U.S.S. Lollipop", NULL, NULL};
s.weapons = calloc(10, sizeof *(s.weapons));
s.weapons[0] = (struct weapon){"Some giant lazer", 4988};
s.engines = calloc(10, sizeof *(s.engines));
s.engines[0] = (struct engine){"Some fuckin engine", 9};
s.engines[1] = (struct engine){"Some other fuckin engine", 10};
s.engines[2] = (struct engine){"Some piece of shit that we don't know what else to call it", -2};

describe_ship(&s);
set_engines_power(&s, 999);
describe_ship(&s);

free(s.engines);
free(s.weapons);

return 0;
}


The point being that since you care so goddamn much about your ship parts, just fucking separate them out to begin with.

I know what you're saying now. You're saying ``But Anon, what if somebody wants to add a DeflectorShield object and a Sensor object and all these other types of objects. Do you really want me to add new LinkedList objects to my ship for every single type just to handle these?'' Well, yes! Or rather, I want you to sanely organize your component hierarchy from the beginning. Even if you stuff them all together like you have now for the sake of type ignorance, your damn universal Visitor object is going to look like

public static interface ShipPartVisitor
{
void visit(ShipEngine shipEngine);
void visit(ShipWeapon shipWeapon);
void visit(DeflectorDish deflectorDish);
void visit(SickBay sickBay);
void visit(Lounge lounge);
// ...
void visit(ObservationPodEjectButtonLocatedOnCaptainsChairOnBridge o);
}

And guess what? Polymorphism isn't free! So enjoy having your VisitorWhichRefillsTheCaptainsPersonalFishTank walk through every other item on your goddamn ship every time that fish needs its water changed.

``Oh, Anon, that's no problem!'' you say. ``I'll just use option 2: separate everything into a type hierarchy so that the visitor visits generic objects and uses instanceof''. So now you have instanceof checks littering every single Visitor you write, and all those checks are running slower than the polymorphic version because you aren't as good at type-checking as the engineers that implemented the language.

Furthermore, if you still are dead-set on cramming all your objects into one collection space and separating them into groups at run-time for every single action, how would you handle something as simple as ``find out how many engines are currently installed on the ship''? If you separate your hierarchy correctly from the beginning, it's trivial. return allEngines.size();. If you don't have properly segregated components, you're going to have to type-check every salt-shaker in Ten Forward every time you bring up the weapon selection menu!

``Oh, you don't understand,'' you say. ``I do have a sane hierarchy set up internally, this is just the external interface, for when other people want to do things to my internal components. I just don't want to have a getAllEngines() method for security reasons.'' I have two responses to that.

First, you're an idiot. Return an Iterator that doesn't implement remove().

Second, you've admitted now that there is actually no need for your ShipPartVisitor interface. What you actually want are ShipEngineVisitors and ShipWeaponVisitors. The visitor pattern in general makes me want to vomit, but if you want to use it on your ship, you can do so without a mile-long interface or instanceof checks everywhere. If you want a thing that visits both the engines and the weapons, you just implement both and call visitEngines(foo); visitWeapons(foo);. (Note that this does not reintroduce the run-time polymorphism issue, because your accept(...){...visit(this);} methods are declared and executed in the context of knowing exactly what the class of the visitee is, so the work can be done at compile-time.)