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.
-
"Is [something] a code smell" is not an answerable question without including criteria that specifies what constitutes a "code smell." Can you ask something more specific? Note: don't replace "code smell" with some equally ambiguous tautology like "best practice."Robert Harvey– Robert Harvey10/04/2018 17:56:13Commented Oct 4, 2018 at 17:56
-
@RobertHarvey - How about now?user315575– user31557510/04/2018 18:01:52Commented Oct 4, 2018 at 18:01
-
"Acceptable" is a tautology. It suffers the same problem as "best practice:" there's no way to determine what is acceptable or not without stating some criteria for evaluation. You might be better off asking questions like why.Robert Harvey– Robert Harvey10/04/2018 18:03:00Commented Oct 4, 2018 at 18:03
-
@RobertHarvey - Let's see if I can fix this. :Duser315575– user31557510/04/2018 18:03:45Commented Oct 4, 2018 at 18:03
-
@RobertHarvey - In a way I did state the criteria, the Clean Code sample is my criteria. Why is that sample considered fine, using an internal attribute to filter out objects, but when I use a map to filter based on attributes, it's all of a sudden considered code smell.user315575– user31557510/04/2018 18:05:30Commented Oct 4, 2018 at 18:05
1 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.