How to refactor an application with multiple switch cases?

Question

I have an application which takes an integer as input and based on the input calls static methods of different classes. Every time a new number is added, we need to add another case and call a different static method of a different class. There are now 50 cases in the switch and every time I need to add another case, I shudder. Is there a better way to do this.

I did some thinking and came up with this idea. I use the strategy pattern. Instead of having a switch case, I have a map of strategy objects with the key being the input integer. Once the method is invoked, it will look up the object and call the generic method for the object. This way I can avoid using the switch case construct.

What do you think?

Answer 1

There are now 50 cases in the switch and every time I need to add another case, I shudder.

I love polymorphism. I love SOLID. I love pure object oriented programming. I hate to see these given a bad rep because they get applied dogmatically.

You have not made a good case for refactoring to strategy. The refactoring has a name by the way. It's called Replace Conditional with Polymorphism.

I've found some pertinent advice for you from c2.com:

It really only makes sense if the same or very similar conditional tests are repeated often. For simple, seldom-repeated tests, replacing a simple conditional with the verbosity of multiple class definitions, and likely moving all this far from the code that actually requires the conditionally required activity, would result in a textbook example of code obfuscation. Prefer clarity over dogmatic purity. -- DanMuller

You have a switch with 50 cases and your alternative is to produce 50 objects. Oh and 50 lines of object construction code. This is not progress. Why not? Because this refactoring does nothing to reduce the number from 50. You use this refactoring when you find you need to create another switch statement on the same input somewhere else. That's when this refactoring helps because it turns 100 back into 50.

So long as you're referring to "the switch" like it's the only one you have, I don't recommend this. The only advantage to come from refactoring now is that it reduces the chances that some goofball will copy and paste your 50 case switch.

What I do recommend is looking closely at these 50 cases for commonalities that can be factored out. I mean 50? Really? You sure you need that many cases? You might be trying to do to much here.

Answer 2

A map of strategy objects alone, which is initialized in some function of your code, where you have several lines of code looking like

     myMap.Add(1,new Strategy1());
     myMap.Add(2,new Strategy2());
     myMap.Add(3,new Strategy3());

requires you and your colleagues to implement the functions/strategies to be called in separate classes, in a more uniform manner (since your strategy objects will all have to implement the same interface). Such code is often a little bit more comprehensive than

     case 1:
          MyClass1.Doit1(someParameters);
          break;
     case 2:
          MyClass2.Doit2(someParameters);
          break;
     case 3:
          MyClass3.Doit3(someParameters);
          break;

However, it still will not release you from the burden of editing this code file whenever a new number needs to be added. The real benefits of this approach is a different one:

• the initialization of the map now becomes separated from the dispatch code which actually calls the function associated to a specific number, and the latter does not contain those 50 repetitions any more, it will just look like myMap[number].DoIt(someParameters). So this dispatch code does not need to be touched whenever a new number arrives and can be implemented according to the Open-Closed principle. Moreover, when you get requirements where you need to extend the dispatch code itself, you will not have to change 50 places any more, but only one.

• the content of the map is determined at run-time (whilst the content of the switch construct is determined before compile-time), so this gives you the opportunity to make the initialization logic more flexible or extendable.

So yes, there are some benefits, and this is surely a step towards more SOLID code. If it pays off to refactor, however, is something you or your team will have to decide by itself. If you do not expect the dispatch code to be changed, the initialization logic to be changed, and the readability of the switch is not a real problem, then your refactoring might not be so important now.

Answer 3

I am strongly in favor of the strategy outlined in the answer by @DocBrown.

I am going to suggest an improvement to the answer.

The calls

 myMap.Add(1,new Strategy1());
 myMap.Add(2,new Strategy2());
 myMap.Add(3,new Strategy3());

can be distributed. You don't have to go back to the same file to add another strategy, which adheres to the Open-Closed principle even better.

Say you implement Strategy1 in file Strategy1.cpp. You can have the following block of code in it.

namespace Strategy1_Impl
{
   struct Initializer
   {
      Initializer()
      {
         getMap().Add(1, new Strategy1());
      }
   };
}
using namespace Strategy1_Impl;

static Initializer initializer;

You can repeat the same code in every StategyN.cpp file. As you can see, that will be a lot of repeated code. To reduce the code duplication, you can use a template which can be put in a file that is accessible to all the Strategy classes.

namespace StrategyHelper
{
   template <int N, typename StrategyType> struct Initializer
   {
      Initializer()
      {
         getMap().Add(N, new StrategyType());
      }
   };
}

After that, the only thing you have to use in Strategy1.cpp is:

static StrategyHelper::Initializer<1, Strategy1> initializer;

The corresponding line in StrategyN.cpp is:

static StrategyHelper::Initializer<N, StrategyN> initializer;

---

You can take the use of templates to another level by using a class template for the concrete Strategy classes.

class Strategy { ... };

template <int N> class ConcreteStrategy;

And then, instead of Strategy1, use ConcreteStrategy<1>.

template <> class ConcreteStrategy<1> : public Strategy { ... };

Change the helper class to register Strategys to:

namespace StrategyHelper
{
   template <int N> struct Initializer
   {
      Initializer()
      {
         getMap().Add(N, new ConcreteStrategy<N>());
      }
   };
}

Change the code in Strateg1.cpp to:

static StrategyHelper::Initializer<1> initializer;

Change the code in StrategN.cpp to:

static StrategyHelper::Initializer<N> initializer;