Getting rid of `instanceof`

Question

In a sprite based game I'm writing, each field in a 2D grid contains a stack of sprites. Mostly the top one counts.

In the rules module of the game, I have a lot of code like this:

public boolean isGameWon(Board board) {
    for (Point point : board.getTargetPoints())
        if(!(board.getTopSpriteAt(point) instanceof Box))
            return false;
    return true;
}

---

Upadate: //Do something counts if there is a Box on top of each Target. I don't see how that can be done with simply adding doSomething() to Sprite, unless doSomething() returns 1 if the sprite is a box, and 0 otherwise. (and that would be just the same as instanceof).

---

I know instanceof is considered harmful because it kills the idea of object oriented programming.

I'm however not sure how to fix the code in my case. Here are some thoughts I've had:

• I don't think it makes it any better to simple add a isABox() method to the Sprite interface.
• Would it help if Box was an interface, so other classes could get the same priviledge?
• Should I try to do something fancy like pattern matching / double dispatch, with visitor like patterns?
• Is it OK that the rules module work intimately with the types, simply because it is supposed to know their semantics anyway?
• Is the entire idea of a rules module strategy pattern flawed?
• It doesn't make sense to build the rules into the Sprites, as they would then all have to be changed when a new type is added.

I hope you have tried something similar and are able to point me in the right direction.

Answer 1

Use polymorphism:

class Sprite {
    ..
    someMethod(){
    //do sprite
    }
    ..
}

class Box extends Sprite {
    ..
    @Overrides
    someMethod(){
    //do box
    }
    ..
}

So, you just need to call sprite.someMethod() in your example.

Answer 2

Instanceof: (Almost) Always Harmful

I took a look at all the answers to your post and tried to understand what you were doing. And I have come to the conclusion that instanceof is exactly what you want and your original code sample was fine.

You clarified that:

• You are not violating the Liskov substitution principle since none of the Box code invalidates the Sprite code.

• You are not forking the code with the response to instanceof. This is why people say instanceof is bad; because people do this:

  if(shape instanceof Circle) {
      area = Circle(shape).circleArea();
  } else if(shape instanceof Square) {
      area = Square(shape).squareArea();
  } else if(shape instanceof Triangle) {
      area = Triangle(shape).triangleArea();
  }
  

  This is the reason why people avoid instanceof. But this is not what you are doing.

• There is a one-to-one relationship between Box and winning the game (no other Sprites can win the game). So you are not in need of an additional "winner" sprite abstraction (because Boxes == Winners).

• You are simply checking the board to make sure that each top item is a Box. This is exactly what instanceof is designed to do.

Everyone else's answer (including my own) adds an additional mechanism for checking whether a Sprite is a Box. However they do not add any robustness. In fact you are taking features which are already supplied by the language and reimplementing them in your own code.

Tomas Narros argues that you should distinguish in your code between "semantic types" and "java types". I disagree. You have already established that you have a java type, Box, which subclasses Sprite. So you already have all the information that you need.

In my view, having a second independent mechanism which also reports "I am a Box", violates DRY (Don't Repeat Yourself). This means not having two independent sources for the same piece of information. You now have to maintain an enum and a class structure.

The so-called "benefit" is the ability to pirouette around a keyword which fully fills the purpose, and is harmful when used in more harmful ways.

---

The golden rule is Use your head. Don't obey rules as hard fact. Question them, learn why they are there, and bend them when appropriate.

Answer 3

Here is my try. Consider defining an enum with the different Sprite types:

class Sprite {
    public enum SpriteType {
         BOX, CAT, BOTTLE, HUMAN, TARGET, /* ... */, SIMPLE;
    }

    public SpriteType getSpriteType(){
       return SIMPLE;
    }
}


class Box extends Sprite {
    @Override
    public SpriteType getSpriteType(){
       return Sprite.SpriteType.BOX;
    }
}

And at last:

public boolean isGameWon(Board board) {
    for (Point point : board.getTargetPoints())
        if(board.getTopSpriteAt(point).getSpriteType()!=SpriteType.BOX)
            return false;
    return true;
}

This way, you can solve the problem of having to create an isATypeX() method in Sprite for every type X. If you need a new type, you add a new value to the enum, and only the rule who need to check this type will need to aknowledge it.

Answer 4

Basic overloading is the way to go here. It's the Sprite class hierarchy that should know what to do and how to do it, as in:

interface Sprite {
    boolean isCountable();
}


class MyOtherSprite implements Sprite {
    boolean isCountable() {
        return false;
    }
 }

 class Box implements Sprite {
    boolean isCountable() {
        return true;
    }
}

int count = 0;
for (Point point : board.getTargetPoints()) {
    Sprite sprite = board.getTopSpriteAt(point);
    count += sprite.isCountable() ? 1 : 0;
}

EDIT: Your edit to the question does not fundamentally change the problem. What you have is some logic that is only applicable to Box. Again, encapsulate that particular logic in the Box instance (see above). You could go further and create a generic superclass for your sprites that defines a default value for isCountable() (note that the method is similar to the isBox one but is actually better from a design perspective, since it makes no sense for a Circle to have a isBox method - should Box also contain a isCircle method?).