3

The use of instanceof or getClass() is largely considered code smell. Is using a variable to indicate the type of object you're using also considered code smell?

Suppose if I had an enum called WeaponType:

public enum WeaponType {
    ReloadableWeapon // -- imagine more weapon types
}

and Weapon class:

public abstract class Weapon 
{
    private WeaponType wt;

    public Weapon(WeaponType wt)
    {
       this.wt = wt;
    }
}

public ReloadableWeapon extends Weapon{

     public ReloadableWeapon()
          super(WeaponType.ReloadableWeapon);
     {

     }
}

In this example, I'm using an enum to determine the type of weapon, essentially, I'm doing with the enum what I can do with instanceof or getClass().

I can check if the weapon is a certain type and proceed, for example, suppose in my game I allow the player to view their weapons based on type in a quick view while under attack.

I can collect all the weapons like so:

List<Weapon> reloadableWeapons = new ArrayList<Weapon>();

for (Weapon weapon : inventory){
     if weapon.istypeof(WeaponType.ReloadableWeapon){
          reloadableWeapons.add(weapon);
     }
}

// code to render reloadableWeapons to UI

Of course this doesn't have to be an enum, it could be any variable, String or int but it's purpose is to indicate what type of object you have.

Notice, I'm not using the enum to check the Weapon and downcast, or controlling behavior. I simply want to single out weapons of a specific type and perform an action, in this case, display in a UI.

5 Answers5

5

When your weapon types enum just mirrors the class hierarchy, then this is a blatant violation of the DRY principle - you encode the same information, the type of a weapon redundantly in two places - one place is the class name itself, one place the enum. That's definitely not just a code smell, that is bad code.

Introducing such a type just to avoid instanceof formally does not make your code better - quite the opposite, you just "reinvent" instanceof and call it istypeof, with all the same drawbacks of the former: whenever you add a new Weaponsubclass, you have to check all places in code with an istypeof statement, if the code is still correctly dealing with the new type. There is no difference in this as if you would use instanceof for it, just an additional disadavantage: whenever you add a new subclass, you will also have to make sure you don't forget to extend the enum.

The situation may be different when you use the enum differently, for example, for modeling types of weapons in a finer or coarser granularity than your class hierarchy (or if you do not use a class hierarchy at all, just a type field). But in general, I recommend trying to achieve orthogonality, your code stays more maintainable if enums do not have implicit, hidden dependencies to something like a class hierarchy, that will become error prone sooner or later.

Doc Brown
  • 218,378
1

Your example is different from using getClass, you have but one generic class and you enum is just a member that tells the type of weapon (not the type of class). If your weapon/game is simple enough (your weapon just strikes) this is a valid approach.

It gets different when you start adding weapon type specific behavior to your generic weapon and you get lots of if statements and switch statements, checking your enum to get to the desired behavior.

Martin Maat
  • 18,652
0

Like anything, it becomes a code smell when it starts to make the code harder to understand and maintain. It can be OK in small doses, or where there is some useful differentiating feature. For example, if some types of weapons can be composed of other types of weapons, but some can't, then it might make sense to have an enum, string, or method that says, "contains sub weapons" or something like that. In most cases anything you can do to a weapon, you can also do to a weapon composed of other weapons. But there are things you can't do with a non-composed weapon (such as break it down into its components) that you can do with a composed weapon.

It might be better to use polymorphism for this case. You could have a base class that does something like this, for example:

public abstract class Weapon 
{
    public reloadWeapon() 
    { 
        /* Do nothing in the base class */ 
    }
};

public class ReloadableWeapon extends Weapon
{
    public reloadWeapon()
    {
        /* Do whatever is necessary here */
    }
};

For non-reloadable weapons, the base class simply returns. For reloadable weapons, it reloads them as necessary.

user1118321
  • 4,981
0

The "right" way to do it in OOP world is the Visitor pattern.

Otherwise I agree with Doc Brown's answer that introducing explicit variable adds other points of failure, but does not give any benefit. Particularly, it does not remove the code smell which is explicit request for the object type. So I think you should not do it. An important exception would be the case when you have to interoperate with another language, reflecting the hierarchy, in which case you may have to introduce the enum anyway.

max630
  • 2,605
0

This is pretty much the same as checking the class, and has the same problems.

What will you do when you add more classes of weapons? Say you have Weapon, ReloadableWeapon (guns), MeleeWeapon (swords), ConsumableWeapon (bombs) and PlaceableWeapon (laser turrets).

If you go with an enum-based or class-based approach to distinguishing them, you will find yourself adding endless combination classes, and then forgetting to check them. You may wish to add cattle prods as a new class ReloadableMeleeWeapon, but then forget to check for ReloadableMeleeWeapon in the reloading code. Or you add it to the reloading code, but then forget to update the code that spawns weapons so that they start full of ammo, so that reloadable weapons start full of ammo, but reloadable melee weapons don't.

Stack Exchange Broke The Law
  • 5,417