Switch vs Polymorphism

Question

I know this is a classic problem but it's so hard to chose the right way or the best. Let me present you a simplified version of the code.

Design 1 using switch :

public class Disease {
  private Color color;
}

public class City {
   private String name;
   private int yellowCubes;
   private int redCubes;
   private int bluesCubes;
   private int blackCubes;

   public void setCubesOnCity(int cubesNb, Disease disease) {
       cubesNb = computed again ..
       disease.subCubes(cubesNb);
       switch(disease.getColor()) {
           case Colors.BLACK:
               blackCubes += cubesNb;
           //other cases
       }
  }

  public int getCubesOnCity(Disease disease) {
      int cubesNb = 0;
      switch(disease.getColor()){
          case Colors.BLACK:
              cubesNb = this.getBlackCubes();
          // Etc
      }
      return cubesNb;
   }

 }

Design 2 with polymorphism:

public abstract class Disease {
   abstract int getCubesSetOn(City city);
   abstract void setCubesOn(City city, int cubesNb);
}

// One of sub classes
public class BlueDisease extends Disease {

  // Always needed for UI
  public Color getColor(){return Colors.BLUE;}

  @Override
  int getCubesSetOn(City city) {
      return city.getBlueCubes();
  }

  @Override
  void setCubesOn(City city, int cubesNb) {
      int totalCubes = cubesNb + city.getBlueCubes();
      city.setBlueCubes(totalCubes);
  }
}

public class City {
   private String name;
   private int yellowCubes;
   private int redCubes;
   private int bluesCubes;
   private int blackCubes;

   public void setCubesOnCity(int cubesNb, Disease disease) {
       cubesNb = computed again ..
       disease.subCubes(cubesNb);
       disease.setCubesOn(this, cubesNb);
  }

  public int getCubesOnCity(Disease disease) {
       return disease.getCubesSetOn(this);
  }
 }

Now for the additional information. There are four diseases (four colors). It's a fixed requirement for the game with no real need for a design where it's easy to refactor if we need to add a new disease.

The problem is that I've heard so many contradictory things like switch is a code smell try to use the Command pattern (polymorphism). Don't create useless classes just to access polymorphism (favor switch or command ?). Favor composition over inheritance (switch wins because Color is included in Disease). I'm not used to the Command pattern but it doesn't seem to fit for my needs.

I know that in the grand scheme of things I achieve the same things with the two designs but this a project where I try to improve myself on design. So I would like to have opinions and advice on what should I prefer when I encounter this type of problem...

Thanks you.

Answer 1

switch, or big if elseif blocks are usually considered a code smell. ie there is probably a better solution.

In you case I would have something like..

Dictionary<colour, int> cubeCounts;
...
cubeCounts[disease.Colour] ++;

Answer 2

Biggest code smell in switch is due to testing. You break the O in SOLID principles - Open Closed Principle.

Why? You have a class that contains the switch. You test the class and it turns out good. Now, later the requirements change and you need a new case. But you've tested this class! It shouldn't be manipulated anymore. It should be open for extensions, but closed for modification (your second example).

The second example is better because it promotes smaller classes that have SRP, that are easier to test (or to create via test driven development) and once tested you are certain that they work and can be reused. To quote Uncle Bob from the linked page:

Classes that must be modified to accommodate new changes are fragile, rigid, unpredictable and unreusable. By insulating the class from changes, the class becomes robust, flexible, and reusable. Also as no modifications are made to the code no bugs can be introduced, leading to code that only becomes more stable over time through testing. The ability to reuse a class that has been working for years without modification is clearly preferable to modifying the class every time requirements change.

Answer 3

In your case, "switch vs. polymorphism" is the wrong question. To a large degree, the only difference between various diseases seems to be the colour. So if you have information that depends on the colour, you can use a map, or you can use an enum value for each colour and use arrays indexed by that enum value.

Answer 4

The thing with both desings you've proposed is that you have mixed responsibilities.

In a "good" design, the City and the Disease Level would be decoupled from each other so that managing disease levels would be performed handled separately from the rest of the data in City class.

Technically, since Disease level is nothing but a counter over a relatively fixed set of integers, you could technically implement this using a simple int array like this:

public enum Color { BLACK, BLUE, RED, YELLOW }

public interface Disease {
    Color getColor();
}

public class DiseaseLevel {
    private final int[] diseaseLevels;
    public DiseaseLevel() {
        diseaseLevels = new int[Color.values().length];
    }

    public int getDiseaseLevel(Disease disease) {
        return diseaseLevels[disease.getColor().ordinal()];
    }

    public void setDiseaseLevel(Disease disease, int level) {
        assert level >= 0 : "Disease level can not be negative!";
        diseaseLevels[disease.getColor().ordinal()] = level;
    }
}

Your City might look like this:

private enum InfectionResult {
    ok, Infected, OUTBREAK
}

public class City {
    private final String name;
    private final long population;

    private final Color color;
    private final DiseaseLevel diseaseLevel;

    public City(String name, long population) {
        this.name = name;
        this.population = population;
        this.diseaseLevel = new DiseaseLevel();
    }

    public String getName() { return name; }
    public long getPopulation() { return population; }

    public Color getColor() { return color; }
    public int getDiseaseLevel() {
        return diseaseLevel.getDiseaseLevel(color);
    }

    private void setDiseaseLevel(int level) {
        diseaseLevel.setDiseaseLevel(color, level)
    }

    public InfectionResult increaseDiseaseLevel(int number) {
         int newLevel = getDiseaseLevel() + number;
         if (newLevel > 3) {
             setDiseaseLevel(3);
             return InfectionResult.OUTBREAK;
         }

         setDiseaseLevel(newLevel);
         return InfectionResult.Infected;
    }

    public InfectionResult decreaseDiseaseLevel(int number) {
         int newLevel = getDiseaseLevel() - number;
         if (newLevel < 0) {
             setDiseaseLevel(0);
             return InfectionResult.ok;
         }

         setDiseaseLevel(newLevel);
         return InfectionResult.Infected;
    }
}

This design currently is less than ideal in that it still mixes soe concerns on the City level, but without knowing the exact requirements (or rules of the game), I baked them in.

Also, I assumed the rules of Pandemic for rules on increasing and decreasing the disease level in a city. It might be that your particular game has different rules or that those rules might be more flexible. Adjust as needed ;)

PS -- for Pandemic, each city may only have cubes of a single color disease, so the whole DiseaseLevel class is overengineered and a single disease counter would suffice perfectly.

Answer 5

What are your requirements? Does this code have a short shelflife? Will you need to add new diseases/colours? Are other people going to be reading your code or is this just a project you're working on alone? How critical is the design of this aspect of your system to the overall picture?

If you're hell-bent on using OO patterns here, also consider:

• the visitor pattern
• template pattern

Both of these leave the implementation to subordinate classes, but also allow a significant amount of control to be centralised. So, could you perhaps write it on way using either of these patterns, and switch (no pun intended) another easily if you dislike it?