When is it acceptable to use instanceof? [closed]

Question

I'm designing a game. In the game, various game objects extend different interfaces (and one abstract class) depending on what they need to be doing, and are passed to handlers which take care of items with a specific interface at defined intervals (they actually spread all their work out in a neat sort of way to make sure input/video/etc is always processed).

Anyway, some of these objects extend the abstract class Collider and are passed to a CollisionHandler. The Collider class and handler take care of everything technical involved in collision, and just ask that an object implement a collidesWith(Collider c) function, and modify itself based on what it has collided with.

Objects of many different classes will be colliding with one another, and will act very differently depending on what type of object they have collided with and its specific attributes.

The perfect solution seems to be to use instanceof like so:

class SomeNPC extends Collider{
    collidesWith(Collider c){
        if(c instanceof enemy){
            Fight it or run away depending on your attributes and theirs.
        }
        else if(c instanceof food){
            Eat it, but only if it's yellow.
        }
        else if(c instanceof BeamOfLight){
            Try to move towards its source.
        }
    }
}

This actually seems like a legitimate place for instanceof. I just get this bad feeling. Like how if a goto made sense in some particular situation. Does the design feel fundamentally off to anyone? If so, what would you recommend doing to achieve the same behavior.

Answer 1

It's strange no one have posted a "not broken" visitor pattern implementation yet. And by not broken I mean not relying on the side effects of the visitor. To do that, we need our visitors to return some result (let's call it R):

interface ColliderVisitor<R> {
     R visit(Enemy e);
     R visit(Food f);
     R visit(BeanOfLight bol);
     R visit(SomeNpc npc);
}

Next, we modify accept to accept the new visitors:

interface Collider {
    <R> R accept(ColliderVisitor<R> visitor);
}

the concrete implementations of collider will have to call the proper visit method, like this (I'm assuming Food implements Collider, but this is not necessary):

class Food implements Collider {
    @Override
    <R> R accept(ColliderVisitor<R> visitor) {
        return visitor.visit(this);
    }
}

Now to implement the collisions we can do something like this:

class SomeNpcCollisionVisitor implements ColliderVisitor<Action> {
    SomeNpcCollisionVisitor(SomeNpc me) { this.me = me; }
    SomeNpc me;
    @Override
    Action visit(Enemy they) { 
        return fightItOrRunAway(me.attributes(), they.attributes());
    }
    @Override
    Action visit(Food f) {
        return f.colour()==YELLOW ? eat(f) : doNothing;
    }
    @Override
    Action visit(BeamOfLight l) {
        return moveTowards(l.source());
    }
    @Override
    Action visit(SomeNpc otherNpc) {
       // What to do here? You did not say! The compiler will catch this thankfully.
    }
}

class CollisionVisitor implements 
        ColliderVisitor<ColliderVisitor<Action>> { // currying anyone?

    @Override
    Action visit(Enemy they) { 
        return new EnemyCollisionVisitor(they); // what to do here?
    }
    @Override
    Action visit(Food f) {
        return new FoodCollisionVisitor(f); // what to do here?
    }
    @Override
    Action visit(BeamOfLight l) {
        return new BeamOfLightCollisionVisitor(l); // what to do here?
    }
    @Override
    Action visit(SomeNpc otherNpc) {
       return new SomeNpcCollisionVisitor(otherNpc);
    }
}

Action collide(Collider a, Collider b) {
    return b.accept(a.accept(new CollisionVisitor()));
}

You can see that the compiler can help you find all the places where you forgot to specify the behaviour. This is not a liability as some people claim, but advantage because you can always disable it by using a default implementation:

class ColliderVisitorWithDefault<R> implements ColliderVisitor {
    final R def;
    ColliderVisitorWithDefault(R def) { this.def = def; }
    R visit(Enemy e) { return def; }
    R visit(Food f) { return def; }
    R visit(BeanOfLight bol) { return def; }
    R visit(SomeNpc npc) { return def; }
}

You will also want some way to reuse the code for collisions of (Food, SomeNpc) and (SomeNpc, Food), but this is outside the scope of this question.

If you think this is too verbose -- that's because it is. In languages featuring pattern matching this can be done in several lines (Haskell example):

data Collider = 
    Enemy <fields of enemy>
  | Food <fields of food>
  | BeanOfLight <fields>
  | SomeNpc <fields>

collide (SomeNpc npc) (Food f) = if colour f == YELLOW then eat npc f else doNothing
collide (SomeNpc npc) (Enemy e) = fightOrRunAway npc (npcAttributes npc) (enemyAttributes e)
collide (SomeNpc npc) (BeamOfLight bol) = moveTowards (bolSource bol)
collide _ _ = undefined -- here you can put some default behaviour

Answer 2

This is a use of instanceof that will make most people cringe. Here is a useful link that may provide you with some insight: http://www.javapractices.com/topic/TopicAction.do?Id=31

Instead, a Collider should have some sort of collide() method that each subclass will override.

Answer 3

You can use a visitor pattern here where subclasses of Collider would implement a separate method for each type of collision it can encounter. So you method might turn into:

class SomeNPC extends Collider {

    public void collidesWith( Enemy enemy ) {}

    public void collidesWith( Food food ) {}

    public void collidesWith( Bullet bullet ) {}

    public void doCollision( Collider c ) {
        if( c.overlaps( this ) ) {
            c.collidesWith( this );
        }
    }
}

You get the idea. The strange thing in your model is that a Collider base class has to know about all potential subclasses in order to define a method for that type. Part of this has to do with the problem of the visitor pattern, but it's also because Collider is combined into Visitor. I'd suggest looking for a separation of the visitor and collider so you can define how you want to behave when collisions happen. What that means to your colliders is they can change how they behave to a collision based on internal state. Say they are invulnerable vs. normal mode, hidden, or dead. Looking at the client code it might be:

collider1.getCollisionVisitor().doCollision( collider2 );
collider2.getCollisionVisitor().doCollision( collider1 );

Answer 4

The visitor class is often recommended. With Visitor you implement a visit method:

interface Visitor {
 void visit(Enemy e);
 void visit(Food f);
 void visit(BeanOfLight bol);
}

But this is in fact equivalent to:

class SomeNPC extends Collider {
  public void collidesWith( Enemy enemy )
  public void collidesWith( Food food )
  public void collidesWith( Bullet bullet )
}

Both of these have disadvantages.

1. You have to implement all of them, even if the response of you object is the same in each case
2. If you add a new kind of object to collide with you have to write a method to implement colliding with it for every single object.
3. If one object in your system reacts differently to 27 kinds of colliders, but everything else reacts the same way, you still have to write 27 visitor methods for every class.

Sometimes the easiest way is to do:

collidesWith(Object o) {
  if (o instanceof Balloon) {
    // bounce
  } else {
    //splat
  }

It has the advantage that it keeps the knowledge of how an object reacts to things it hits with that object. it also means that if Balloon has subclasses RedBalloon, BlueBalloon etc we don't have to take that into account, as we would do with the visitor pattern.

The traditional argument for not using instanceof is that it isn't OO, and you should be using polymorphism. However you might be interested in this article: When Polymorphism Fails, by Steve Yegge which explains why instanceof is sometimes the right answer.

Answer 5

There are a few ways to handle it this:

• enums for the collider types, slightly ugly but fail-proof
• class dispatch with handlers: i.e. something like Map<Class, CollisionHandler>, you pick the CollisionHandler off the passed collider class (or enum type) and call processCollision(source). Each Colllider class has its own map of handlers.
• Based on the above you can also create a combination of Colliders types and write handlers for, that's something like Map<Pair<ColliderType>, CollisionHandler> for each new collision type you'd need to create a new handler. The plus side is that such declarations can be external (dependency injection), so new NPCs/Object can be added along with the Collision handlers.

Anyways first you make sure it works the way you feel the most comfortable with and you can refactor it later.

Answer 6

This might be something to consider. Using instanceof can be useful but another way to consider the visitor idea is to build a Factory class that examines a mutual field to determine what type the visitor is.

You still have to build new classes for each implementation, but they are routed in an abstract class that defines a method

 public abstract class Qualifier{
 public abstract String type(); //...
 }

/**
 * QualifierFactory method.
 */
public static Qualifier getQualifier(Item sp, Config cf) {
    String type = cf.type();
    if (XQualifier.type().equals(type))
        return new XQualifier(sp, cf);
    else if (NQualifier.type().equals(type))
        return new NQualifier(sp, cf);
    else if (Tools.isNone(type) || NoneQualifier.type().equals(type))
        return new NoneQualifier(sp);
    else if (CSQualifier.type().equals(type))
        return new CSQualifier(sp, cf);//...
 }

The returned object could be the action .