Jaki jest lepszy sposób na ucieczkę od zbyt wielu, jeśli / else-if z następującego fragmentu kodu?

14

Próbuję napisać serwlet, który wykonuje zadania na podstawie wartości „action” przekazanej mu jako dane wejściowe.

Oto próbka tego

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
        */

     }
    }

Wszystko, czego potrzebuję, to uciec od zbyt wielu sprawdzeń w moim kodzie w celu poprawienia czytelności i zachowania kodu. Więc wypróbowałem inne alternatywy, takie jak

1. zmień skrzynkę - nadal wykonuje zbyt wiele kontroli przed wykonaniem mojej czynności

2. odbicie

i] jedną z najważniejszych rzeczy jest bezpieczeństwo - które pozwala mi na dostęp nawet do zmiennych i metod w klasie pomimo jej specyfikatora dostępu - nie jestem pewien, czy mógłbym użyć jej w kodzie

ii] a drugim jest to, że zajmuje to więcej czasu niż zwykłe kontrole if / else-if

Czy jest jakieś lepsze podejście lub lepszy projekt, który ktoś mógłby zaproponować, aby lepiej zorganizować powyższy kod?

EDYTOWANE

Dodałem odpowiedź na powyższy fragment, biorąc pod uwagę poniższą odpowiedź .

Ale nadal następujące klasy „ExecutorA” i „ExecutorB” wykonują tylko kilka wierszy kodu. Czy dobrą praktyką jest dodawanie ich jako klasy niż dodawanie ich jako metody? Proszę doradzić w tym zakresie.

Tom Taylor
źródło
1
Możliwy duplikat podejść do sprawdzania wielu warunków?
komara
2
Dlaczego przeciążasz pojedynczy serwlet 9 różnymi czynnościami? Dlaczego nie po prostu przypisać każdej akcji do innej strony, wspieranej przez inny serwlet? W ten sposób wybór akcji jest dokonywany przez klienta, a kod serwera koncentruje się tylko na obsłudze żądania klienta.
Maybe_Factor

Odpowiedzi:

13

W oparciu o poprzednią odpowiedź Java pozwala, aby wyliczenia miały właściwości, dzięki czemu można zdefiniować wzorzec strategii, coś w rodzaju

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

Wtedy twoja Executor(strategia) byłaby

public interface Executor {
    foo execute();
}

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

I wszystkie twoje doPostmetody if / else stają się czymś podobnym

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

W ten sposób możesz nawet użyć lambdów dla wykonawców w wyliczeniach.

J. Pichardo
źródło
dobrze powiedziane .. Ale potrzebuję małego wyjaśnienia .. Wszystkie moje akcje action1 (), action2 () byłyby kilkoma liniami kodu .. czy dobrze byłoby spakować go w klasie?
Tom Taylor
4
Nie jest to liczba wierszy, które powinny cię przekonać do tworzenia określonych klas / obiektów, ale fakt, że reprezentują one różne zachowania. 1 pomysł / koncepcja = 1 obiekt logiczny.
mgoeminne,
2
@RajasubaSubramanian Możesz również użyć referencji lambda lub metody, jeśli uważasz, że klasa jest zbyt ciężka. Executorjest (lub może być) funkcjonalnym interfejsem.
Hulk,
1
@ J.Pichardo Dziękuję za aktualizację :) Ponieważ wciąż jestem w java7, nie mogłem użyć wyrażenia lambda .. Więc poszedłem zgodnie z implementacją enum wzorca strategii sugerowanego tutaj na dzone.com/articles/strategy-pattern-implemented
Tom Taylor
1
@RajasubaSubramanian spoko, nauczyłem się czegoś nowego,
J. Pichardo
7

Zamiast używać refleksji, użyj dedykowanego interfejsu.

tj. zamiast:

      /**
       * 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);  

Posługiwać się

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

Implementuje każdą z nich dla każdej akcji, a następnie:

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

Oczywiście to rozwiązanie nie jest najlżejsze, więc może nie będziesz musiał podchodzić do tej długości.

Walfrat
źródło
myślę, że musi być ProcessActionzamiast Czy ActionProcessto tak ...?
Tom Taylor
1
Tak, naprawiłem to.
Walfrat
1
I, bardziej ogólnie, odpowiedź brzmi „użyj mechanizmów OOP”. Dlatego tutaj powinieneś opisać „sytuację” i związane z nią zachowanie. Innymi słowy, reprezentowanie logiki przez obiekt abstrakcyjny, a następnie manipulowanie tym obiektem zamiast leżących u jego podstaw nakrętek i śrub.
mgoeminne,
Naturalne rozszerzenie podejścia zaproponowanego przez @Walfrat polegałoby na zaproponowaniu (abstrakcyjnej) fabryki, która tworzy / zwraca właściwą ProcessAction w zależności od określonego parametru String.
mgoeminne,
@mgoeminne To brzmi dobrze
J. Pichardo
2

Użyj wzorca poleceń , będzie to wymagało interfejsu poleceń podobnego do tego:

interface CommandInterface {
    CommandInterface execute();
}

Jeśli Actionssą lekkie i tanie w budowie, użyj metody fabrycznej. Załaduj nazwy klas z pliku właściwości, który mapuje actionName=classNamei użyj prostej metody fabrycznej, aby zbudować akcje do wykonania.

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

Jeśli działania są drogie w budowie, użyj puli, takiej jak HashMap ; jednakże w większości przypadków sugerowałbym, że można tego uniknąć zgodnie z zasadą pojedynczej odpowiedzialności, delegując kosztowny element do pewnej wstępnie zbudowanej wspólnej puli zasobów zamiast samych poleceń.

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

Można je następnie wykonać za pomocą

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

Jest to bardzo solidne i oddzielone podejście, które stosuje SRP, LSP i ISP zasad SOLID . Nowe polecenia nie zmieniają kodu mapującego polecenia. Polecenia są łatwe do wdrożenia. Można je po prostu dodać do projektu i pliku właściwości. Polecenia powinny zostać ponownie wprowadzone, co czyni go bardzo wydajnym.

Martin Spamer
źródło
1

Można użyć obiektu opartego na wyliczaniu, aby zmniejszyć konieczność twardego kodowania wartości ciągu. Zaoszczędzi ci to trochę czasu i sprawi, że kod będzie bardzo fajny do czytania i rozszerzania w przyszłości.

 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;
      }
  }
Karan
źródło
1

Wzorzec metody fabrycznej jest tym, co wyglądam, jeśli szukasz skalowalnego i mniej konserwowalnego projektu.

Wzorzec metody fabrycznej definiuje interfejs do tworzenia obiektu, ale pozwól, aby podklasa zdecydowała, którą klasę utworzyć. Metoda fabryczna pozwala klasie odroczyć tworzenie instancji do podklasy.

 abstract class action {abstract doStuff(action)}

action1, action2 ........ actionN konkretna implementacja za pomocą metody doStuff Implementacja rzeczy do zrobienia.

Zadzwoń

    action.doStuff(actionN)

Więc w przyszłości, jeśli zostaną wprowadzone dalsze działania, wystarczy dodać konkretną klasę.

Anuj Singh
źródło
literówka abstarct -> streszczenie w pierwszym wierszu kodu. Proszę edytować. Czy możesz również dodać nieco więcej kodu, aby wypłukać ten przykład, aby pokazać bardziej bezpośrednio, w jaki sposób odpowiada on na pytanie PO?
Jay Elston,
0

W odniesieniu do @J. Odpowiedź Pichardo Piszę modyfikację powyższego fragmentu w następujący sposób

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();
  }
}
Tom Taylor
źródło
Czy nie tworzysz zbyt wielu klas, jeśli jest zbyt wiele akcji? Czy mamy lepszą implementację?
Vaibhav Sharma