Is it good practice to often use instanceof?

Question

The scenario. I'm writting game-related code. In that game a Player(its also a class) has a list of Item. There are other types of items that inherit from Item, for example ContainerItem, DurableItem or WeaponItem.

Obviously it is very conveniant for me to just have List<Item>. But when I get the players items, the only way for me to distinguish between what type of item is by using the instanceof keyword. I'm sure I've read that reliaing on it is bad practice.

Is it ok to use it in this case? Or should I rethink all of my structure?

Answer 1

Let's say I am writing some inventory code:

public void showInventory(List<Item> items) {
    for (Item item : items) {
        if (item instanceof ContainerItem) {
            // container display logic here
        }
        else if (item instanceof WeaponItem) {
            // weapon display logic here
        }
        // etc etc
    }
}

That will compile and work just fine. But it misses out on a key idea of object oriented design: You can define parent classes to do general useful things, and have child classes fill in specific, important details.

Alternate approach to above:

abstract class Item {
    // insert methods that act exactly the same for all items here

    // now define one that subclasses must fill in themselves
    public abstract void show()
}
class ContainerItem extends Item {
    @Override public void show() {
        // container display logic here instead
    }
}
class WeaponItem extends Item {
    @Override public void show() {
        // weapon display logic here instead
    }
}

Now we have one place to look, the show() method, in all our child classes for inventory display logic. How do we access it? Easy!

public void showInventory(List<Item> items) {
    for (Item item : items) {
        item.show();
    }
}

We are keeping all the item-specific logic inside specific Item subclasses. This makes your codebase easier to maintain and extend. It reduces the cognitive strain of the long for-each loop in the first code sample. And it readies show() to be reusable in places you haven't even designed yet.

Answer 2

It's ok if it's easy for you to understand.

Moving branching logics from where they naturally belong all to subclasses is not necessarily a good idea. It may create the wrong dependency; it may cause bloated classes, and it may be hard to navigate and understand.

In the end, it's all about how to physically organize our code with multiple dimensions of concerns in a one dimensional space. It's not a trivial problem, it is subjective, and there is no panacea.

In particular, our industry inherited a lot of rules of thumbs that were made in the last century based on technical and economical constraints of that era. For example, tooling were very limited; programmers were highly expensive; applications evolved slowly.

Some of these rules may no longer apply today.

Answer 3

I don't necessarily think instanceof is bad for coders who know what they are doing and use it to avoid having to write more complicated code to get around it. There is a use for everything, and also a mis-use.

With that said, the description you provide does not require instanceof. There are various ways you can implement this without instanceof, and (the most important thing) the alternate solution must be better than using instanceof. Don't just go with a non-instanceof solution to avoid instanceof because you heard it is bad.

I think that for your scenario an non-instanceof solution has benefits in making the solution more easily extensible.

for (Item i: items) {
    if (ItemTypes.WEAPON_ITEM.equals(i.getType)) {
        processWeaponType(i);
    }

}

or

for (Item i: items) {
    if (WeaponItem.class.equals(i.getType)) {
        processWeaponType(i);
    }

}

Answer 4

You should rethink your structure, instanceof in non-meta code is almost always a sign for an anti-pattern. Try to define the behaviour all Items have in common (like having a picture, a description and something happening when you click on them) in the Item-class/interface, making use of the abstract-keyword if appropiate, and then use polymorphism to implement the specifics.

Answer 5

IMHO using instanceof is a code smell. Simply put - it makes your code procedural, not object oriented. The OO way of doing this is using the visitor pattern.

The visitor pattern also allows you to easily build decorators and chain of responsibility on top of it, thus achieving separation of concerns, which results in shorter, cleaner and easier to read and test code.

Also do you really need to know the exact class ? Cant you take advantage of polymorphism ? After all Axe IS a Weapon just as Sword is.

Answer 6

You should rethink maybe and try to use polymorphism to implement your List<Item> idea.

Here is some references for your problem that can probably help :

• Prefer polymorphism over instanceof and downcasting
• instanceof versus getClass in equals Methods (Interview with Josh Bloch)
• Polymorphism and Interfaces (see section about When to use instanceof)

---

(References from Is instanceof considered bad practice? If so, under what circumstances is instanceof still preferable? )