Should I use Polymorphism for simple switch statement?

Viewed 274

I am new to cleaning code/refactoring and I learned to avoid having long switch statements like the one I have below. I discovered that polymorphism is a helpful technique to shorten complex switch statements. Would polymorphism be a good idea for this simple switch statement?

String periodValue;
int numberOfDataPoints;

getNumberOfDataPoints(String selectedGraphType) {
    switch (selectedGraphType) {
      case "1D": 
        {
          periodValue = "300";
          numberOfDataPoints = 289; 
        }
        break;
      case "5D": 
        {
          periodValue = "1800";
          numberOfDataPoints = 241;
        }
        break;
      case "1M": 
        {
          periodValue = "86400";
          numberOfDataPoints = 31;
        }
        break;
      case "1Y": 
        {
          periodValue = "259200";
          numberOfDataPoints = 123;
        }
        break;
  }

}

4 Answers

Perhaps this will help you get started with creating a base Graph class which all your other Graph types can extend. Along with a factory class that generates a Graph object based on the given string passed in.

public class Graph
{
    public String periodValue;
    public int dataPoints;
    
    public Graph()
    {
        periodValue = "0";
        dataPoints = 0;
    }
    
    public void setPeriodValue(String newValue)
    {
        periodValue = newValue;
    }
    
    public void setDataPoints(int newDataPoints)
    {
        dataPoints = newDataPoints;
    }
    
    public String getPeriodValue()
    {
        return periodValue;
    }
    
    public int getDataPoints()
    {
        return dataPoints;
    }
}

public class OneDayGraph extends Graph
{
    public OneDayGraph()
    {
        periodValue = "300";
        dataPoints = 289;
    }
}

//add more Graph classes that extend your base class, Graph

public class GraphFactory
{
    public Graph createGraph(String typeOfGraph)
    {
        switch (typeOfGraph)
        {
            case "1D": 
            {
                return new OneDayGraph();
            }
            //add more cases for other graph types...
            default:
            {
                System.out.println("No known string to Graph Class for " + typeOfGraph);
                break;
            }
        }
        
        return new Graph();
    }
}

Then you could create a factory object, and give it the particular string to create your desired Graph object

GraphFactory factory = new GraphFactory();
Graph oneDay = factory.createGraph("1D");
System.out.println("The periodValue is " + oneDay.getPeriodValue());
System.out.println("The dataPoints is " + oneDay.getDataPoints());

You don't need polymorphism you can just use a class that combines the two values and a Map to omit the switch statement.

public class GraphType {
   // made the fields public final and omitted the getters for simplicity.
   // add getters if needed

   public final int periodValue;
   public final int numberOfDataPoints;

   GraphType(int periodValue, int numberOfDataPoints) {
      this.periodValue = periodValue;
      this.numberOfDataPoints = numberOfDataPoints;
   }

}

then build the map

Map<String, GraphType> graphTypeByName = new HashMap<>();

graphTypeByName.put("1D",new GraphType(300, 289));
graphTypeByName.put("5D",new GraphType(1800, 241));
graphTypeByName.put("1M",new GraphType(86400, 31);
graphTypeByName.put("1Y",new GraphType(259200, 123);

and use the map

public GraphType getNumberOfDataPoints(String selectedGraphType) {
    GraphType graphType = graphTypeByName.get(selectedGraphType);

    if(graphType == null) {
       // handle graphType not registered. Default value or exception ?
    }

    return graphType;
}

This method is more flexible than using an enum. The default case is also easier to test. I think that the GraphType is not an enum. An enum is a set of fixed names that are unlikely to change, like NORH, SOUTH, EAST, WEST or HOUR, MINUTES, SECONDS.

If you want to ensure that a only the GraphTypes that you want exist and no others can be instantiated, implement a registry like this:

public class GraphTypeRegistry {

    public class GraphType {
       public final int periodValue;
       public final int numberOfDataPoints;

       private GraphType(int periodValue, int numberOfDataPoints) {
           this.periodValue = periodValue;
           this.numberOfDataPoints = numberOfDataPoints;
       }
   }

   private Map<String, GraphType> graphTypeByName = new HashMap<>();

   public GraphTypeRegistry(){
      graphTypeByName.put("1D",new GraphType(300, 289));
      graphTypeByName.put("5D",new GraphType(1800, 241));
      graphTypeByName.put("1M",new GraphType(86400, 31);
      graphTypeByName.put("1Y",new GraphType(259200, 123);
   }

   public GraphType getGraphType(String graphTypeName) {
      GraphType graphType = graphTypeByName.get(graphTypeName);

      if(graphType == null) {
         // handle graphType not registered. Default value or exception ?
      }

      return graphType;
  }
} 

The biggest problem of that code is the lack of type safety. This method can only use 4 particular strings, but accepts any string, without giving any clue to its callers which strings are accepted, or giving any feedback to callers if they provide a wrong string.

One way to improve this would be to use an enum instead. This also allows you to store additional data about values in the enum itself:

public enum GraphType {
  day(300),
  week(1800),
  month(86400),
  year(259200);

  public final int periodValue;

  GraphType(int periodValue) {
    this.periodValue = periodValue;
  }
}

Then, your method to calculate the number of data points can become:

getDataPoints(GraphType selectedGraphType) {
  int period = selectedGraphType.periodValue;
  // find datapoints depending on period
}

I would write that like below.

public enum GraphType {
  GT_1D(300, 289),
  GT_5D(1800, 241),
  GT_1M(86400, 31),
  GT_1Y(259200, 123);

  public final int periodValue;
  public final int numberOfDataPoints;

  GraphType(int periodValue, int numberOfDataPoints) {
    this.periodValue = periodValue;
    this.numberOfDataPoints = numberOfDataPoints;
  }
}

Or, if there is some formula hidden behind periodValue and numberOfDataPoints, I would write that formula in the constructor or method.

Related