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

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.