Why is using an internal attribute to filter objects considered OK, but when I use a map to filter based on attributes, it's considered a code smell?

Question

A long time ago I asked about using an enum to essentially use as a poor version of instanceof to make decisions about an object.

As stated in this answer:

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.

I then wondered about having a List or a Map that contained a collection of attributes that the object could hold and using a method, I would ask it about what attributes it contained and then proceed to work with that object.

Let's put the Collection idea aside for now, because I want to focus on another relatable problem.

In Clean Code, P.19 the following code snippet is given as a good example of well named methods and variables.

public List<Cell> getFlaggedCells(){
    List<Cell> flaggedCells = new ArrayList<Cell>();
    for(Cell cell : gameboard){
         if(cell.isFlagged()){
              flaggedCells.add(cell);
         }
    }
    return flaggedCells;
}  

From this code snippet, we know a know that a cell contains a method that verifies if it's been flagged. In other words, we can ask the cell, are you flagged?

We then collect a List of flagged cells and return to the caller.

In my second question, I had the following code snippet:

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. 
    }
}

A Wizard class might look like this:

public Wizard extends Character {

    private List<Weapons> weapons;

    public boolean tryAdd(Weapon weapon){
       if !(weapon.containsAttribute(WeaponType.LongBlade)){
          return weapons.add(weapon);
       }
       return false;     
    }
}

Much like the Clean Code snippet, I'm asking the object, do you contain a specific attribute, and if not, add it to the weapons collection.

As stated in this link, using an enum to map polymorphic behaviors is a code smell, and rightfully so. The same warning is given in my second link at the top.

Question:

Why is the Clean Code sample considered fine, using an internal attribute to filter out objects, but when I use a map to filter based on attributes, it's considered code smell?

I'm not checking for a specific attribute and then calling a method to perform an action, I'm simply filtering, nothing more, nothing less.

Answer 1

I don't see a problem with your code because there is a clear distinction between a Weapon and a Weapon Attribute.

Your WeaponType enum does not appear to directly model the weapon class hierarchy. WeaponType.LongBlade could be an attribute of Kitana or Machete.

You would obviously have different game behavior based on the type of weapon used, and I think you are correct in using weapon attributes to do this.

Maybe I'm missing something, but I think what you have is fine.