4

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.

  • 1
  • 2
    Even though the four diseases is a fixed requirement, you might consider neither of the two options and going for a Map or array "indexed by the color" instead, with count as the value. If there were 400 fixed diseases, would you have 400 private members? A Map makes it easy to query, and you have direct access to total number of cubes using `size()'. – senevoldsen Apr 15 '17 at 16:20
  • 1
    Though not responding to your original question, I notice that you have the notion of cubes, which while it accurately depicts the board game version of pandemic ;), it may not reflect the visuals that your version provides and thus not be the best name. Infections might be a better name, but it's best to avoid naming based off of implementation/design details if you can. – unflores Apr 20 '17 at 8:20
  • 1
    This is the well-known expression problem. If you expect your application to get new data-types in the future while the set of operations on those types remains constant, then use polymorphism. If you expect your collection of data types to remain constant, use switch. If you expect you are going to add new data types and operations, use the visitor pattern. – Giorgio Apr 20 '17 at 10:39
17

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] ++;
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.

  • Yep, I also prefer the second design because I can easily see how it's more robust if requirements are changing, but my teammates were a bit reluctant and one even made me doubt. Thanks for the refreshing on OCP :) – Tirke Apr 15 '17 at 18:26
  • 3
    The second designs also violates OCP, just uses a different technique. The switch has been moved to the polymorphic disease classes, but still requires that City defines the necessary methods, like setBlueCubes, and getBlueCubes. – senevoldsen Apr 15 '17 at 18:55
2

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.

0

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?

  • Yep, this is not in a very important part of the project but still ... Also, no new disease to be added. We are working on this project in a team of four but they rely on me (maybe a bit too much) for the design. I'm not really hell-bent on anything but I need to look a bit closer at the visitor pattern because it's quite common and I've never used it outside of an interpreter and I surely didn't implemented it well at that time. Thanks. – Tirke Apr 15 '17 at 18:08
0

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.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.