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.

Improve this question
17
  • 1
    I hope you realize that many people consider the use of words like "code smell" as highly insulting. At least those who realise how language can be used to demonise opposing opinions. Commented Jul 1, 2018 at 12:09
  • One could argue that ReloadableWeapon should not be a type at all. Your base Weapon class should have an isReloadable flag, default to false... Likewise for every attribute you'd want to filter weapons on... I won't actually make that argument. It's an argument I would flesh out pretty thoroughly if this were my design though. Commented Jul 1, 2018 at 15:37
  • @gnasher729 - It is highly insulting and frustrating because people are quick to throw that term around. It's almost as if everything thing you write is "code smell". It makes me wonder sometimes, why bother writing something when another developer will consider it code smell anyway. Commented Jul 1, 2018 at 15:39
  • @svidgen - I don't think Weapon should worry if it's reloadable or not. What about other types? This means Weapon is going to change every time i add a new type? Commented Jul 1, 2018 at 15:41
  • @sveta Not necessarily. Could apply the same principal and put those attributes in a dict/map or something, which is more of what I had in mind, but didn't really consider you're working in Java. (No duck/dynamic typing, right?) Commented Jul 1, 2018 at 15:44

5 Answers 5

Reset to default
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.

Improve this answer
6
  • In the context that I'm using it, to display weapons of a specific type in the UI, nothing more, is it still a code smell? i.e I'm not using it to control behavior of the Weapon (checking if it's reloadable, then calling reload()). Commented Jun 30, 2018 at 17:39
  • 2
    @Sveta: in that context, using isinstanceof would probably be ok. Violating the DRY principle is not. Commented Jun 30, 2018 at 17:40
  • Bare with me here, how would I be violating the DRY principal? I know this goes beyond the scope of the question, but in the interest of learning :D. You can edit your answer rather an explain in the comments. Commented Jun 30, 2018 at 17:42
  • @Sveta: clearer now? Commented Jun 30, 2018 at 20:22
  • 1
    @Sveta: let me add one note: using instanceof is not necessarily a bad thing, "code smell" just means "the code is not generic any more and will not be automatically deal with new subtypes". For example, if your UI deals specificially with "reloadable weapons", with a static field or columns explicitly for this, using instanceof is ok. If, however, you want to create a generic UI, which adapts automatically to new weapon subtypes, then using instanceof has a high risk of causing issues. So if it is are the first situation, don't overthink this and use instanceof. Commented Jun 30, 2018 at 20:55
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.

Improve this answer
4
  • In your second point, you mean if Weapon had a method called reload(), it can get difficult if I'm using the enum/getClass/instanceof to check the type and call the method? Is this an indication that Weapon needs to be redesigned? Commented Jun 30, 2018 at 17:07
  • @Sveta As your weapons gets more complex and get specific behavior you should turn your generic weapon class into an abstract base class and make a sub class for each weapon. Then you no longer need your enum. Commented Jun 30, 2018 at 17:45
  • That's exactly what's shown in my example right now, Weapon is abstract, my issue is, in places where I might have used instanceof or getClass() to single out weapons of a specific type, is it okay to use an enum or another other type of variable instead? In this context, I want to grab all weapons that can reload to display in the UI for the player. I'm not using the enum to control behavior of the object. Commented Jun 30, 2018 at 18:11
  • @Sveta I did not read your code well enough, sorry for that. In this case, a virtual property named CanReload would be more approptiate. I am not a Java guy, it may have to be a method, I don't know, you get the point. Commented Jun 30, 2018 at 18:28
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.

Improve this answer
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.

Improve this answer
2
  • 1
    Doesn't the visitor introduce a lot of code for the simple task for displaying all weapons of a specific type? I don't want to access the method of a specific type, I just want to display them in the UI, not control behavior of the weapons Commented Jun 30, 2018 at 18:42
  • it does. There are arguments out there in internet. I am not quite convinced myself. I just pointed out the popular answer to the problem Commented Jul 1, 2018 at 14:39
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.

Improve this answer
3
  • I agree with this answer. My method is just a poor way to avoid instanceof. Given my scenario, is the use of instanceof okay? What should I do when I need the class to tell me what it is? Remember, I'm not downcasting to access a method, I simply want to display all the weapons that are the same type in a UI. For example, imagine a quick view when you press a shortcut that brings up all your reloadable weapons while you're under attack, I need the game to distinguish between Reloadables and non reloadables and display them. Commented Jul 1, 2018 at 15:33
  • So why not call isReloadable()? Commented Jul 1, 2018 at 23:57
  • 1
    I could, or I could use the approach mentioned in the comments above and have a list of attributes for each weapon, so a ReloadableWeapon will obviously have Reloadable as an attribute, as well as portable, etc. I like the list approach, it's flexiable makes the code more maintainable. Commented Jul 2, 2018 at 4:48

Your Answer

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