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);
}

}

I puked a little in my mouth.

Would you make different lists for every type of parts, use instanceof ShipEngine, or use a visitor?

I would use Lisp.

>>2
that's a side effect of the visitor pattern
>>3
can you give an implementation of the spaceship in lisp?

Here's an alternative I put on the old /prog/, it doesn't need visitors or instanceOf, but it's not how inheritence ``should be used".
import java.util.LinkedList;
import java.util.List;

public class SpaceShip
{

// this is a hack...
public static interface ShipPart
{
void setEnginePower(int powerLevel);

void setWeaponDamage(int damage);
}

/*
* No-op for every methods, it is left to the concrete parts to implement the methods they want.
*/
public static class DefaultShipPart implements ShipPart
{

@Override
public void setEnginePower(int powerLevel)
{
// no-op is default
}

@Override
public void setWeaponDamage(int damage)
{
// no-op is default
}

}

public static class ShipEngine extends DefaultShipPart
{
public int power = 0;

@Override
public void setEnginePower(int newPower)
{
System.err.println("ENGINE POWER MODIFIED : " + power + " TO " + newPower);
power = newPower;
}

}

public static class ShipWeapon extends DefaultShipPart
{
public int damage = 5, cooldown = 2;

@Override
public void setWeaponDamage(int newDamage)
{
System.err.println("WEAPON DAMAGE MODIFIED : " + damage + " TO " + newDamage);
damage = newDamage;
}

}

// 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)
{
for (ShipPart pickedPart : parts)
pickedPart.setEnginePower(powerLevel);
}

public void setWeaponsDamage(final int damage)
{
for (ShipPart pickedPart : parts)
pickedPart.setWeaponDamage(damage);
}

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);

// power the weapons !

myShip.setWeaponsDamage(2000);
}

}

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.)

>>6
Nice post anon, here are my thoughts...

The purpose of ship parts is that the parts will often be treated as a group (that's what I assumed, but it may be wrong); for example, when the ship is shot, the shot part would be desactivated, whatever its type is. These methods that work with all the parts as a group would have to be modified everytime I add a new type of ship part, because I'd add a new list for those.

I thought about the parts count for a type, and yes, i would need to browse the whole part list to get this count; that sucks.

So I guess it all depends on what you usually do with the parts. If you often have to treat the independently, it would be better to use separate list for each ship part. Else, if you often treat the parts as a whole group, it would be better use a visitor for the rare cases where you must do operation based on type.

In this example, I think most operations are based on type, so splitting the ship parts in separate lists probably is a better idea.

thanks anon!

>>6

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!

Instant classic!

I congratulate you on your excellent explanation (I wanted to explain to >>1 but I wouldn't have had the time to go in such amazing detail). You should go teach programming for a living (unless you already are).