22

In my application I have a 2d array of entities to represent a grid. Each location in the grid can either be empty or occupied by an entity (in this case it's just a person or wall). Right now I use instanceof to check whether an entity is a person or a wall.

I was thinking of giving each entity a method which returns an enum stating their type so i.e. a wall entity would return EntityType.WALL. I was wondering if this is the best idea to remove the use of instanceof or is instanceof suitable in this scenario?

3
  • 2
    To me, the enum solution doesn't sound much better from the software design perspective than the instanceof solution. You can probably solve this much more elegantly using subtype polymorphism (i.e., create subclasses and override some methods with Person/Wall/Empty-specific behaviors). The other option is to use dynamic dispatch, which is achieved in a limited way (double-dispatch) through the visitor pattern. I think there are now answers below addressing each of these approaches.
    – DaoWen
    Nov 22, 2015 at 23:18
  • 1
    Note: there's a typo in the title. But you can't fix it because there is already a question named "Avoiding instanceof in Java". I suggest you reword the title to be more specific.
    – Tunaki
    Nov 23, 2015 at 11:02
  • Possible duplicate of Avoiding instanceof in Java
    – Thomas
    Nov 23, 2015 at 13:02

7 Answers 7

42

Use Tell, Don't Ask: instead of asking the objects what they are and then reacting on that, tell the object what to do and then walls or people do decide how they do what they need to do.

For example:

Instead of having something like this:

public class Wall {
    // ...
}

public class Person {
    // ...
}

// later
public class moveTo(Position pos) {
    Object whatIsThere = pos.whatIsThere();
    if (whatIsThere instanceof Wall) {
         System.err.println("You cannot move into a wall");
    }
    else if (whatIsThere instanceof Person) {
         System.err.println("You bump into " + person.getName());
    }
    // many more else branches...
}

do something like this:

public interface DungeonFeature {
    void moveInto();
}

public class Wall implements DungeonFeature {
    @Override
    public void moveInto() {
        System.err.println("You bump into a wall");
    }

   // ...
}

public class Person implements DungeonFeature {
    private String name;

    @Override
    public void moveInto() {
        System.err.println("You bump into " + name);
    }

    // ...
}

// and later
public void moveTo(Position pos) {
    DungeonFeature df = currentPosition();
    df.moveTo(pos);
}

This has some advantages.

First, you don't need to adjust a giant if then else tree each time you add a new dungeon feature.

Second, the code in the dungeon features is self-contained, the logic is all in the said object. You can easily test it and move it.

1
  • 1
    I like this answer—but I think you need to expand it a bit. Maybe you can add a really simple example to illustrate your point.
    – DaoWen
    Nov 22, 2015 at 23:41
9

The theoretical solution to removing the instanceof in a refined way is the usage of the Visitor Pattern. How it works is that the object that needs to know whether the other element is a wall or person calls that object with itself as a parameter, and that particular object calls back thus providing information about its type.

Example,

public class Person {
    void magic() {
        if(grid.getAdjacent() instanceof Person) {
            Person otherPerson = (Person)grid.getAdjacent();
            doSomethingWith(otherPerson);
        } else if(grid.getAdjacent() instanceof Wall) {
            Wall wall = (Wall)grid.getAdjacent();
            doOtherThingWith(wall);
        }
    }
}

Can become

public class Person extends Entity {
    void magic() {
        grid.getAdjacent().visit(this);
    }

    void onVisit(Wall wall) {
        doOtherThingWith(wall);
    }

    void onVisit(Person person) {
        doSomethingWith(person);
    }

    public void visit(Person person) {
        person.onVisit(this);
    }
}

public class Wall extends Entity { 
    public void visit(Person person) {
        person.onVisit(this);
    }
}
1
  • 1
    What about the Empty case? (I know it probably seems obvious to you that you can add an empty case, but if the OP has never seen the Visitor Pattern before, it might not be so obvious to them.)
    – DaoWen
    Nov 22, 2015 at 23:21
1

I would let person and wall inherit from a abstract superclass ( e.g. Tile ) which has a method getType() returning an enum or int and implement this method in Wall and Person returning the appropriate

6
  • Would this be ok? public abstract class Entity { public abstract EntityType getEntityType(); } public class Wall extends Entity { private static final EntityType type = EntityType.PEDESTRIAN; @Override public EntityType getEntityType() { return type; } }
    – John Radke
    Nov 22, 2015 at 23:11
  • Instead of using inheritance, it would be better to use an interface (e.g. Renderable) here, because neither a Wall nor a Person is a Tile. Nov 22, 2015 at 23:18
  • Stupid 5 minute rule sorry ~ Is there a way I can force the classes that implement Entity to have a EntityType field and which cannot be null? If I add the field to the abstract class a class could extend Entity but not set an EntityType if that makes sense.
    – John Radke
    Nov 22, 2015 at 23:19
  • @MickMnemonic why isn't it a tile? It is in a grid - but you are right with the interface - this is also an valid option
    – ligi
    Nov 22, 2015 at 23:29
  • @JohnRadke you might use the "@NonNull" annotation
    – ligi
    Nov 22, 2015 at 23:30
1

If you follow the other answers here and implement a visitor pattern or use an enum you will not make a mistake.

However, it might also help to think about what exactly it is you want to do with that switching logic (be it instanceof or visitors), because sometimes there is a simpler way to do that.

For example, if all you want to do is check if an entity occupies a grid in a blocking way, then you can just add a method boolean isSolid() to each entity via interface. You can use this with default methods for extra beauty:

public interface GridPhysics {
    default boolean isSolid() {
        return true;
    }

    // other grid physics stuff
}

public class Wall implements GridPhysics {
    // nothing to do here, it uses the default
}

// in your game logic
public boolean canMoveTo(GridPhysics gridCell) {
    return !gridCell.isSolid() && otherChecks();
}

You might also want to have a look at entity component systems (e.g. Artemis), which basically take this idea of "composition over inheritance" to the extreme.

0

ligi's answer is right on the money. (Whoever downvoted it, I wonder what they were thinking.) As an alternative, consider this:

abstract class Tile
{
    public final EntityType type;

    protected Tile( EntityType type )
    {
        this.type = type;
    }
}

abstract class Pedestrian extends Tile
{
    public Pedestrian()
    {
        super( EntityType.PEDESTRIAN );
    }
}

abstract class Wall extends Tile
{
    public Wall()
    {
        super( EntityType.WALL );
    }
}

The rationale behind this is that the "type" of the entity is a permanent characteristic of the entity, so it is suitable to be specified in the constructor and to be realized in a final member field. If it is returned by a virtual method (non-final method in java parlance) then descendants would be free to return one value at one point in time, and another value at another point in time, which would spell havoc.

Oh, and if you really cannot stand the public final member, go ahead and add a getter for it, but my advice would be that never mind the purists, public final members without getters are perfectly fine.

1
  • 3
    You are adding no value by having an enum providing the same information instanceOf provides anyway. The way to solve this is polymorphism and correct use of OO principles.
    – Tim B
    Nov 23, 2015 at 12:48
0

Answers are very good here nothing to say anything about that, but if I were in such situation and if it is allowed than i would have been gone for a 2d int array with possible value 0(for empty by default assignment) and 1,2 for person or wall.

-2

As mentioned on this other question, modern Java compilers are very efficient at operations like instanceof. You should be fine using it.

In fact, one of the other provided answers tested instanceOf and string comparisons, and instanceOf was significantly faster. I recommend you stick with using it.

7
  • 5
    The issue with instanceof isn't an efficiency problem, but rather a software design problem. Using instanceof is considered a "code smell" that indicates you should really be using a better software abstraction to solve your problem (in my experience, usually either subclass polymorphism or dynamic dispatch).
    – DaoWen
    Nov 22, 2015 at 23:15
  • 1
    en.wikipedia.org/wiki/Overengineering - instanceof is a design smell, but so what? YAGNI in my opinion prevails. Does the application do what you need it to do? yes - then you are done; no - what is wrong, perhaps fixing that would involve fixing the code smell, perhaps not.
    – emory
    Nov 22, 2015 at 23:28
  • 2
    @MikeNakis - I wasn't actually the original downvoter. If the criteria for downvoting is simply "this answer isn't useful", then I think this definitely deserves a downvote (the OP never expressed any concern about the efficiency of instanceof). However, I've been under the impression that downvoting actually means "this answer is misleading or wrong". In either case, I think this answer is promoting using a very nasty ad-hoc solution where a more elegant (and probably more efficient!) solution would be to use polymorphism.
    – DaoWen
    Nov 22, 2015 at 23:34
  • 1
    @emory - I think there are situations where instanceof is justified (since using a more "elegant" solution would just lead to unnecessary complexity), but in this case I think Robert's suggestion (i.e., using polymorphism) is simpler (both conceptually and in terms of code complexity), more easily maintainable, and probably more efficient—so sticking with an ad-hoc series of instanceof checks seems like a terrible suggestion to me.
    – DaoWen
    Nov 22, 2015 at 23:39
  • 5
    @EliteMasterEric - Adding a method on each type of object for each behavior has about the same code footprint as the instanceof check for each case—but it has the added conceptual benefit of separating the invariant portion of the algorithm from the variant (type-specific) behaviors. Using inheritance in this way can also allow the compiler to sanity-check for you (e.g., if I add a new "Monster" type that can also occupy a square, the compiler will complain if I don't provide all the methods corresponding to actions, but not so if I use a chain of instanceofs).
    – DaoWen
    Nov 23, 2015 at 0:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.