Using a Collection with Attributes to Check and Make Decisions

Question

I was reading on this SESE page about using a variable to indicate the object type, more specifically, an enum.

The accepted answer states:

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.

As suggested in the comments of the OP, a Collection containing specific attributes might be better. I like this approach because I could query it to find out more about the object and proceed accordingly.

For example:

public enum GameObjectAttributes{
 Replenishable
 Destructable
 Health
 LongBlade
 ShortBlade
 Mana
 Health
 Potion
 Indestructible
}

Weapon:

public abstract class Weapon 
{
 private List<GameObjectAttributes> gOAttributes;
 // imagine a constructor :D.
 public final boolean containsAttribute(GameObjectAttributes attribute)
 {
 // determine if weapon contains a specific attribute. 
 }
}

Suppose I had a Sword object, and the level of detail allows a Sword to be damaged and broken if hit hard enough, so my Sword object might contain Destructable and LongBlade as attributes.

Question:

Does my code suffer from what the answer in the link warns against, that I'm using a variable to map my class hierarchy? Although it might contain an enum about what the object is, it also contains details that for obvious reasons one enum won't be able to tell you, in my Sword example, that it's Destructable.

Answer 1

If we're basing it on strictly what you have written here, no, there is no violation. However, I have to imagine that you have logic somewhere regarding the fact that it is Destructable or that it is a LongSword, am I correct?

In this case, presumably you're using classes to determine behavior for these attributes or else you're using a gigantic if else chain. In the former case, then yes, you're running into precisely the DRY principle violation in which you referred to. In the latter case, you're not, but don't do it this way either. It's not good practice.

However alternatively what you can do is pass a class to the constructor of your enum directly linking the enum with the class which deals with it. Assuming all these attributes can be succinctly generalized into a common interface, you can apply the attributes to the weapon without ever knowing what enum instance you have. This is ideal.

What you want to avoid are things like:

if(gOAttribute.equals(GameObjectAttributes.Destructable)) {
 // Do destructable logic
} else if (gOAttribute.equals(GameObjectAttributes.LongSword)) {
 // Do long sword logic
} else if (...)
 ...

• user315575: Suppose if I were to use the containsAttribute(GameObjectAttributes attribute) to do something other than actions with the object. For example, suppose if I say Wizards cannot carry swords, before adding a Sword a Wizard inventory, I might run a check that says if (!sword.containsAttribute(GameObjectAttribute.LongBlade)){// add code}. Would that be okay?
• user315575: In other words, I'm not checking if it's a LongBlade to determine if I can swing the blade, I have a method for that. Instead I might use it for decisions not applicable to Sword or any specific object. For example, determine who can carry it.
• user315575: Or I can list these attributes when the players mouse hovers over the game object. Could you provide a code example for your ideal situation?
• Neil: @BasementJoe If it isn't a gigantic if else chain, then it's just a bunch of ifs distributed across your code ultimately, and at least one for each enum. Ideally you want to be able to ask each attribute to apply itself without knowing what it is.
• user315575: Could you provide a code sample of how you would do it? Or in other words what you describe in your answer as ideal.

Answer 2

Without knowing more of your specific case, I would say yes, you are doing the same thing, you are just making it more complex.

The fundamental problem you are having, is that you are trying to decide how an object would react to some action from outside the object. This is fundamentally not how object-orientation works (or should work). You should ask the object directly to perform the action, and the object will hopefully tell you what the result is if any.

For example: You say a Wizard can't carry Sword. That might be, but the user can still ask the Wizard to try, can't it? So just implement a tryCarry() method for all characters, and let the character decide whether that is possible.

Second example: Not all objects are destructible you said. Still you could implement a takeDamage() for all objects since you obviously want to deal damage to everything, and let the object decide whether it "destructed", or it had no effect.

In short: Don't try to decide how an object might react to some action from the outside, tell it what happened, and let it deal with it.

• user315575: Bare with me here, I understand to some extent what your saying, but suppose if I choose implement the tryCarry() method, how would that method determine the object is in fact a Sword and reject it? It would have to query it, and ask it, how would it do it?
• user315575: The Sword would have to in some way return true or false that it is in fact a Sword or a LongBlade type weapon.
• Robert Bräutigam: It really depends on the exact requirements and design. Two solutions come to mind: 1. I would think about whether the character class or the weapon class should really be modeled as subclasses. These only make sense if there is really a difference in behavior. 2. Maybe this is one of those rare cases when double dispatching (en.wikipedia.org/wiki/Double_dispatch) is the right solution.