How to avoid instanceof calls?

Question

I defined this simple method:

public static boolean isBorder(int x, int y) throws CollisionDetectionException {
        try {
            if ( (levelItems[x][y] instanceof StaticGameObject && levelItems[x][y].isVisible()) ||
                (levelItems[x-1][y] instanceof StaticGameObject && levelItems[x-1][y].isVisible()) ||
                (levelItems[x][y+1] instanceof StaticGameObject && levelItems[x][y+1].isVisible()) ||
                (levelItems[x][y-1] instanceof StaticGameObject && levelItems[x][y-1].isVisible()) ||
                (levelItems[x-1][y-1] instanceof StaticGameObject && levelItems[x-1][y-1].isVisible()) ||
                (levelItems[x-1][y+1] instanceof StaticGameObject &&levelItems[x-1][y+1].isVisible()) ||
                (levelItems[x+1][y] instanceof StaticGameObject && levelItems[x+1][y].isVisible()) ||
                (levelItems[x+1][y+1] instanceof StaticGameObject && levelItems[x+1][y+1].isVisible()) ||
                (levelItems[x+1][y-1] instanceof StaticGameObject && levelItems[x+1][y-1].isVisible()) ) {
                return true;
            } else {
                return false;
            } 
        } catch (ArrayIndexOutOfBoundsException e) {
            throw new CollisionDetectionException("Collision couldn't be checked because checking position " + x + "/" + y + " caluclated values below (0/0)");
        }
    }

As you can see i have a 2dimensional array. Now i want to check a specific position ((x/y) -> the method arguments), if there are any visible StaticGameObject in the neighbouring fields of the 2dimensional array.

The levelItems array consists of so called GameObject. StaticGameObject is a a direct subclass of GameObject.

Any hints how to improve this method?

Answer 1

Don't make this any more complex. Simply be sure all subclasses of GameObject implement isVisible. That's all it takes.

If you need to also distinguish moveable vs. non-movable, then you need two methods:

• isVisible -- visible or not

• isMovable -- moveable or not

You never need instanceof.

[It's not clear from the question, but it appears that the class StaticGameObject actually means is a subclass of GameObject with isMovable() false.]

Answer 2

Add a method to GameObject

bool isBorderObject() { return false; }

Then override in StaticGameObject

bool isBorderObject() { return true; }

change the test to

(levelItems[x][y].isBorderObject() && levelItems[x][y].isVisible())

Also, the if could be this nested for

for (int i = x-1; i <= x+1; ++i) {
   for (int j = y-1; j <= y+1; ++j) {
       GameObject item = levelItems[i][j];
       if (item.isBorderObject() && item.isVisible()) 
           return true;
   }
}
return false;

Answer 3

This should get rid of the extremely large if block you have:

for(int col = x-1; col <= x+1; col++)
{
    for(int row = y-1; row <= y+1; row++)
    {
        if(levelItems[row][col] instanceof StaticGameObject && levelItems[row][col].isVisible())
            return true;
    }
}

(This solution merely reduces the crazy if, not get rid of the instanceof, as you can see)

Of course, in both my example and yours, you should make sure to check for array bounds problems.

Answer 4

Think about "inversion of control".

As an example, how could introducing this method to the GameObject class:

public boolean
isBorder()
{
    return false;
}

and this override in the StaticGameObject class:

public boolean
isBorder()
{
    return self.isVisible();
}

simplify the code above?

Answer 5

(Disclaimer: I've worked on Java games running on a lot of mobile devices)

People already showed how to simplify all these if statements. I'd add that defining your own CollisionDetectionException is probably completely overkill.

For a start, such low-level Java details don't exist at the OO level: exceptions, especially checked exceptions, are a Java idiosynchrasies that is really not necessary. Some very good and very impressive Java frameworks do mostly away with them, like Spring. Then a lot of very impressive and powerful apps, made of millions and millions of lines of code, run perfectly fine without ever using the concept of checked exception because they're, well, written in language in that don't have this kind of concept (once again: a "checked exception" doesn't exist at the OO level, hence any OOA/OOD to OOP without ever needing to use checked exception).

In any case: there's really no reason to turn an ArrayIndexOutOfBoundException into your own specific checked exception. This means you plan to use exception for flow control, which is a HUGE no-no.

Then, regarding your "isBorder" test... You probably don't need that. You think you do, but you really don't. Why do you want to know if it's a "border"? To detect a collision I guess.

You're using a static method to detect if it's a border, and static is the anti-thesis of OO. What is important is the messages you are passing around between objects. You may have all your objects responding to some isCollidingWith(...) message: objects that are "border" knows they are border, they'll know how to deal with a isCollidingWith(...) message.

Then you can go even further: instead of doing a isCollidingWith(...) you could have some resolveCollisionWith(...) method.

public class Wall implements MySuperAbstraction {

    int wallHitPoints = 42;

    boolean isWallStillUp = true;

    void resolveCollisionWith( SomeObject o ) {
        if ( o.isStrongEnoughToHarmWall ) {
           wallHitPoints--;
           isWallStillUp = wallHitPoints > 0;
        }
    }

}

This is just a quick piece of code and it doesn't deal with the fact that SomeObject needs probably to bounce when he hits the wall, etc. but the point stands: in OO objects knows how to communicate between themselves. They know how to deal with the various messages passed around.

If you want to do OO, then all I can tell you is that Java's static, instanceof and checked exceptions are definitely not the way the go. Basically, they're the anti-thesis of OO :) ]

Answer 6

A revision to @Lou's solution which is to have one method and one loop.

for (int i = 0; i < 9; i++) 
   if (levelItems[x-1 + i/3][y-1 + i%3].isBorderObjectVisible())  
       return true; 
return false; 

Answer 7

Another nice technique is to have a function that returns the set of adjacent cells. In this way you avoid (or anyway move) the double-loop. It's better separation of concerns; when you discover that your algorithm was forgetting about out-of-bounds conditions, there's exactly one place to fix it.

  public Set<GameObject> adjacentItems(int x, int y) {
    Set<GameObject> set = new HashSet<GameObject>();
    for (int i = -1; i < 2; ++i)
      for (int j = -1; j < 2; ++j)
        set.add(levelItems[x+i][y+j]);
    return set;
  }

  public boolean isBorder(int x, int y) {
    for (GameObject item : adjacentItems(x, y))
      if (item.isBorder() && item.isVisible())
        return true;
    return false;
  }