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.

• gnat: Possible duplicate of Approaches to checking multiple conditions?
• Maybe_Factor: Why are you overloading a single servlet with 9 different actions? Why not simply map each action to a different page, backed by a different servlet? That way selection of the action is done by the client and your server code just focuses on serving the client's request.

Answer 1

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.

• Tom Taylor: well said.. But i need a small clarification.. All my actions action1(), action2() would be few lines of code.. will it be good to pack it within a class?
• mgoeminne: This is not the number of lines that should convince you to create specific classes/objects, but the fact that they represent different behaviours. 1 idea/concept = 1 logical object.
• Hulk: @RajasubaSubramanian You could also use a lambda or method reference if you feel that a class is too heavy. Executor is (or can be) a functional interface.
• Tom Taylor: @J.Pichardo Thanks for the update :) Since I'm still in java7 I couldn't use lambda expression.. So, followed the enum implementation of strategy pattern suggested here at dzone.com/articles/strategy-pattern-implemented
• J. Pichardo: @RajasubaSubramanian cool, I learned something new,

Answer 2

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.

• Tom Taylor: i think it must be ProcessAction instead of ActionProcess is that so... ?
• Walfrat: Yes I fixed it.
• mgoeminne: And, more generally, the answer is "use OOP mechanisms". So, here, you should reify the "situation" and its associated behaviour. In other words, representing your logic by an abstract object, and then manipulate this object instead of its underlying nuts and bolts.
• mgoeminne: Also, a natural extension of the approach proposed by @Walfrat would consist in proposing an (abstract) factory that creates/returns the right ProcessAction depending on the specified String parameter.
• J. Pichardo: @mgoeminne That sound about right

Answer 3

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.

Answer 4

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;
 }
 }

Answer 5

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.

• Jay Elston: typo abstarct --> abstract in the first code line. Please edit. Also, can you add a bit more code to flush this example out to show more directly how this answers the OP's question?

Answer 6

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();
 }
}

• Vaibhav Sharma: Aren't you creating too many classes if there are too many actions. Do we have better implementation.

Answer 7

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.