16

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.

2
  • 1
    Possible duplicate of Approaches to checking multiple conditions?
    – gnat
    Mar 6, 2017 at 8:22
  • 2
    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. Mar 15, 2017 at 3:38

6 Answers 6

14

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.

6
  • 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?
    – Tom Taylor
    Mar 6, 2017 at 8:41
  • 4
    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.
    – mgoeminne
    Mar 6, 2017 at 10:29
  • 2
    @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.
    – Hulk
    Mar 6, 2017 at 11:28
  • 1
    @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
    – Tom Taylor
    Mar 7, 2017 at 3:10
  • 1
    @RajasubaSubramanian cool, I learned something new, Mar 7, 2017 at 3:42
7

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.

6
  • i think it must be ProcessAction instead of ActionProcess is that so... ?
    – Tom Taylor
    Mar 6, 2017 at 8:47
  • 1
    Yes I fixed it.
    – Walfrat
    Mar 6, 2017 at 8:49
  • 1
    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
    Mar 6, 2017 at 10:26
  • 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.
    – mgoeminne
    Mar 6, 2017 at 10:32
  • @mgoeminne That sound about right Mar 6, 2017 at 14:50
2

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 built 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.

1

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

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.

1
  • 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?
    – Jay Elston
    Apr 13, 2017 at 1:36
0

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();
  }
}
1
  • Aren't you creating too many classes if there are too many actions. Do we have better implementation. Apr 6, 2020 at 7:34

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.