What is the better way to escape from too many if/else-if from the following code snippet?

Question

I am trying to write a servlet which does task based on the "action" value passed it to as input.

Here is the sample of which

public class SampleClass extends HttpServlet {
     public static void action1() throws Exception{
          //Do some actions
     }
     public static void action2() throws Exception{
          //Do some actions
     }
     //And goes on till action9


     public void doPost(HttpServletRequest req, HttpServletResponse res)throws ServletException, IOException {
          String action = req.getParameter("action");

          /**
           * I find it difficult in the following ways
           * 1. Too lengthy - was not comfortable to read
           * 2. Makes me fear that action1 would run quicker as it was in the top
           * and action9 would run with a bit delay - as it would cross check with all the above if & else if conditions
           */

          if("action1".equals(action)) {
               //do some 10 lines of action
          } else if("action2".equals(action)) {
               //do some action
          } else if("action3".equals(action)) {
               //do some action
          } else if("action4".equals(action)) {
               //do some action
          } else if("action5".equals(action)) {
               //do some action
          } else if("action6".equals(action)) {
               //do some action
          } else if("action7".equals(action)) {
               //do some action
          } else if("action8".equals(action)) {
               //do some action
          } else if("action9".equals(action)) {
               //do some action
          }

          /**
           * So, the next approach i tried it with switch
           * 1. Added each action as method and called those methods from the swith case statements
           */
          switch(action) {
          case "action1": action1();
               break;
          case "action2": action2();
               break;
          case "action3": action3();
               break;
          case "action4": action4();
               break;
          case "action5": action5();
               break;
          case "action6": action6();
               break;
          case "action7": action7();
               break;
          case "action8": action8();
               break;
          case "action9": action9();
               break;
          default:
               break;
          }

          /**
           * Still was not comfortable since i am doing un-necessary checks in one way or the other
           * So tried with [reflection][1] by invoking the action methods
           */
          Map<String, Method> methodMap = new HashMap<String, Method>();

        methodMap.put("action1", SampleClass.class.getMethod("action1"));
        methodMap.put("action2", SampleClass.class.getMethod("action2"));

        methodMap.get(action).invoke(null);  

       /**
        * But i am afraid of the following things while using reflection
        * 1. One is Security (Could any variable or methods despite its access specifier) - is reflection advised to use here?
        * 2. Reflection takes too much time than simple if else
        */

     }
    }

All i need is to escape from too many if/else-if checks in my code for better readability and better code maintanance. So tried for other alternatives like

1.switch case - still it does too many checks before doing my action

2.reflection

i]one main thing is security - which allows me to access even the variables and methods within the class despite of its access specifier - i am not sure weather i could use it in my code

ii] and the other is it takes time more than the simple if/else-if checks

Is there any better approach or better design some one could suggest to organise the above code in a better way?

EDITED

I have added the answer for the above snippet considering the below answer.

But still, the following classes "ExecutorA" and "ExecutorB" does only a few lines of code. Is it a good practice to add them as a class than adding it as a method? Please advise in this regard.

J. Pichardo · Accepted Answer · 2017-03-06T16:34:21.990

Based on the previous answer, Java allows enums to have properties so you could define a strategy pattern, something like

public enum Action {
    A ( () -> { //Lambda Sintax
        // Do A
       } ), 
    B ( () -> executeB() ), // Lambda with static method
    C (new ExecutorC()) //External Class 

    public Action(Executor e)
        this.executor = e;
    }

    //OPTIONAL DELEGATED METHOD
    public foo execute() {
        return executor.execute();
    }

    // Action Static Method
    private static foo executeB(){
    // Do B
    }
}

Then your Executor (Strategy) would be

public interface Executor {
    foo execute();
}

public class ExecutorC implements Executor {
    public foo execute(){
        // Do C
    }
}

And all your if/else in your doPost method become something like

public void doPost(HttpServletRequest req, HttpServletResponse res) throws ServletException, IOException {
    String action = req.getParameter("action");
    Action.valueOf(action).execute();
}

This way you could even use lambdas for the executors in the enums.

Walfrat · Answer 2 · 2017-03-06T08:49:16.153

Instead of using reflection, use a dedicated interface.

ie instead of :

      /**
       * Still was not comfortable since i am doing un-necessary checks in one way or the other
       * So tried with [reflection][1] by invoking the action methods
       */
      Map<String, Method> methodMap = new HashMap<String, Method>();

    methodMap.put("action1", SampleClass.class.getMethod("action1"));
    methodMap.put("action2", SampleClass.class.getMethod("action2"));

    methodMap.get(action).invoke(null);

Use

 public interface ProcessAction{
       public void process(...);
 }

Implements each of them for each actions and then :

 // as attribute
Map<String, ProcessAction> methodMap = new HashMap<String, ProcessAction>();
// now you can add to the map you can either hardcode them in an init function
methodMap.put("action1",action1Process);

// but if you want some more flexibility you should isolate the map in a class dedicated :
// let's say ActionMapper and add them on init : 

public class Action1Manager{
    private static class ProcessAction1 implements ProcessAction{...}

    public Action1Manager(ActionMapper mapper){
       mapper.addNewAction("action1", new ProcessAction1());
    }
}

Of course this solution isn't the lighest, so you may not need to go up to that length.

Martin Spamer · Answer 3 · 2024-07-12T10:49:36.837

Use the Command Pattern, this will require a Command Interface something like this:

interface CommandInterface {
    CommandInterface execute();
}

If the Actions are lightweight and cheap to build then use a Factory Method. Load the classnames from a properties file which maps actionName=className and use a simple factory method to build the actions for execution.

    public Invoker execute(final String targetActionName) {
        final String className = this.properties.getProperty(targetAction);
        final AbstractCommand targetAction = (AbstractCommand) Class.forName(className).newInstance();
        targetAction.execute();
    return this;
}

If the Actions are expensive to build then use a pool, such as a HashMap; however, in most cases, I'd suggest this could be avoided under the Single Responsibility Principle delegating the expensive element to some pre-constructed common resource pool rather than the commands themselves.

    public class CommandMap extends HashMap<String, AbstractAction> { ... }

These can then be executed with

    public Invoker execute(final String targetActionName) {
        commandMap.get(targetActionName).execute();
        return this;
}

This is a very robust and decoupled approach that applies SRP, LSP and ISP of the SOLID principles. New commands do not change the command mapper code. The commands are simple to implement. They can be just added to the project and properties file. The commands should be re-entrant and this makes it very performant.

score 1 · Answer 4 · answered Mar 06 '17 at 08:16

You can utilize the enumeration based object to reduce the need for hardCoding the string values. It will save you some time and makes the code much neat to read & extend in the future.

 public static enum actionTypes {
      action1, action2, action3....
  }

  public void doPost {
      ...
      switch (actionTypes.valueOf(action)) {
          case action1: do-action1(); break;
          case action2: do-action2(); break;
          case action3: do-action3(); break;
      }
  }

Anuj Singh · Answer 5 · 2017-04-13T19:26:55.853

Factory Method pattern is what I looks if you are looking for scalable and less maintainable design.

Factory Method pattern defines a interface for creating an object, but let subclass decide which class to instantiates. Factory Method lets a class defer instantiation to subclass.

 abstract class action {abstract doStuff(action)}

action1,action2........actionN concrete implementation with doStuff Method implementing thing to do.

Just call

    action.doStuff(actionN)

So in future if more action are introduced, you just need to add concrete class.

score 0 · Answer 6 · edited Apr 13 '17 at 12:45

With reference to @J. Pichardo answer i am writing the modifying the above snippet as the following

public class SampleClass extends HttpServlet {

public enum Action {
    A (new ExecutorA()),
    B (new ExecutorB())

    Executor executor;

    public Action(Executor e)
        this.executor = e;
    }

    //The delegate method
    public void execute() {
        return executor.execute();
    }
}

public foo Executor {
    foo execute();
}

public class ExecutorA implements Executor{
   public void execute() {
      //Do some action
   }
}

public class ExecutorB implements Executor{
   public void execute() {
      //Do some action
   }
}

public void doPost(HttpServletRequest req, HttpServletResponse res)throws ServletException, IOException {

  String action = req.getParameter("action"); 
  Action.valueOf(action).execute();
  }
}

score 0 · Answer 7 · answered Jul 12 '24 at 14:38

The switch statement with nine cases is simple, obvious, doesn’t require any extra code, and is easily extended. Note how all the answers actually avoided writing nine cases down. So they are more complex already before even being able to handle nine cases! Since the formatting of your switch cases doesn’t contribute anything, I would even write each case into a single line, identically formatted, so any (incorrect) deviation from the pattern is clearly visible.

If your input is a string, you might consider having one function that maps it to a constant or fails.

What is the better way to escape from too many if/else-if from the following code snippet?

7 Answers7

Linked