9

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?

  • 2
    What is the actual problem with the current code? – Philip Kendall Apr 23 '17 at 5:33
  • What happens when you have to make one of these changes? Do you have to add a switch case and call a pre-existing method in your complex system, or do you have to invent both the method and its call? – Kilian Foth Apr 23 '17 at 5:37
  • @KilianFoth I have inherited this project as a maintenance developer and have not had to make any changes yet. I will however make changes soon, so I want to refactor now. But to answer your question, yes to the latter. – Kaushik Chakraborty Apr 23 '17 at 5:39
  • 2
    I think you need to show a condensed example of what's going on. – whatsisname Apr 23 '17 at 6:09
  • 1
    @KaushikChakraborty: then make up an example from memory. There are situations where a 250+ case uber-switch is appropriate, and there are cases where switch is bad no matter how few cases it has. The devil is in the details and we have no details. – whatsisname Apr 23 '17 at 20:00
12

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.

  • I agree with what you are saying. The code has plenty of redundancies, it could be that a lot of the cases are not even necessary but from a cursory glance it does not seem so. Each of the case calls a method which calls multiple systems and aggregates the results and returns to the calling code. Every class is self contained, does one job and I am afraid I will violate the high cohesion principle, were I to reduce the number of cases. – Kaushik Chakraborty Apr 23 '17 at 8:01
  • 2
    I can get 50 without violating high cohesion and keep things self contained. I just can't do it with one number. I'd need a 2, a 5, and another 5. That's why it's called factoring out. Seriously, look at your whole architecture and see if you can't spot a way out of this 50 case hell you're in. Refactoring is about undoing bad decisions. Not perpetuating them in new forms. – candied_orange Apr 23 '17 at 14:22
  • Now, if you can see a way to reduce the 50 using this refactoring go for it. To leverage off of Doc Browns idea: A map of maps can take two keys. Something to think about. – candied_orange Apr 23 '17 at 15:10
  • 1
    I agree with Candied's comment. The problem is not the 50 cases in the switch statement, the problem is the higher level architectural design that is causing you to call a function that needs to decide between 50 options. I've designed some very large and complex systems and have never been forced into a situation like that. – Dunk Apr 25 '17 at 13:49
  • @Candied"You use this refactoring when you find you need to create another switch statement on the same input somewhere else."Can you elaborate this?As I have a similar case where I have switch cases but on different layers like we have in our project first authorisation, validation, CRUD procedures then dao. So in every layer there is the switch cases on the same input i.e a integer, but doing different functions like auth, valid. so should we create one class fir each type which has different methods? Does our case fit into what you are trying to say by "repeating same switch on same input"? – Siddharth Trikha May 16 '17 at 15:27
9

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.

  • While I am reluctant to blindly replace every switch with polymorphism I will say that using a map the way Doc Brown suggests here has worked very well for me in the past. When you implement the same interface please replace Doit1, Doit2, etc. with one Doit method that has many different implementations. – candied_orange Apr 23 '17 at 14:50
  • And if you have control over the type of the input symbol used as the key, you can go a step further by making doTheThing() a method of the input symbol. Then you don't need to maintain the map. – Kevin Krumwiede Apr 24 '17 at 6:34
  • 1
    @KevinKrumwiede: what you suggest means simply passing the strategy objects themselves around in the program, as a replacement for the integers. However, when the program takes an integer as input from some external data source, there has to be a mapping from the integer to the related strategy at least in one place of the system. – Doc Brown Apr 24 '17 at 6:44
  • Expanding on Doc Brown's suggestion: you could also create a factory that would contain the logic for creating the strategy objects, should you decide to go this way. That said, the answer provided by CandiedOrange makes most sense to me. – Vladimir Stokic Apr 24 '17 at 7:03
  • @DocBrown That's what I was getting at with "if you have control over the type of the input symbol." – Kevin Krumwiede Apr 24 '17 at 7:40
0

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;

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.