Przeprojektowanie funkcji zwracającej kod liczby całkowitej reprezentujący wiele różnych statusów

Odziedziczyłem okropny kod, który zamieściłem poniżej.

• Czy istnieje nazwa tego konkretnego anty-wzoru?

• Jakie są zalecenia dotyczące refaktoryzacji tego?

  // 0=Need to log in / present username and password
  // 2=Already logged in
  // 3=Inactive User found
  // 4=Valid User found-establish their session
  // 5=Valid User found with password change needed-establish their session
  // 6=Invalid User based on app login
  // 7=Invalid User based on network login
  // 8=User is from an non-approved remote address
  // 9=User account is locked
  // 10=Next failed login, the user account will be locked
  
  public int processLogin(HttpServletRequest request, HttpServletResponse response, 
                          int pwChangeDays, ServletContext ServContext) { 
  }
  

Kod jest zły nie tylko dlatego, że magiczne liczby , ale także dlatego, że łączy w sobie kilka znaczeń w kodzie zwrotnym, chowając się w jego znaczeniu, oznaczając błąd, ostrzeżenie, pozwolenie na utworzenie sesji lub kombinację trzech, co czyni go zły wkład w podejmowanie decyzji.

Sugerowałbym następującą refaktoryzację: zwracanie wyliczenia z możliwymi wynikami (jak zasugerowano w innych odpowiedziach), ale dodanie do wyliczenia atrybutu wskazującego, czy jest to odmowa, rezygnacja (pozwolę ci przejść ostatni raz) lub jeśli jest OK (PASS):

public LoginResult processLogin(HttpServletRequest request, HttpServletResponse response, 
                            int pwChangeDays, ServletContext ServContext) { 
    }

==> LoginResult.java <==

public enum LoginResult {
    NOT_LOGGED_IN(Severity.DENIAL),
    ALREADY_LOGGED_IN(Severity.PASS),
    INACTIVE_USER(Severity.DENIAL),
    VALID_USER(Severity.PASS),
    NEEDS_PASSWORD_CHANGE(Severity.WAIVER),
    INVALID_APP_USER(Severity.DENIAL),
    INVALID_NETWORK_USER(Severity.DENIAL),
    NON_APPROVED_ADDRESS(Severity.DENIAL),
    ACCOUNT_LOCKED(Severity.DENIAL),
    ACCOUNT_WILL_BE_LOCKED(Severity.WAIVER);

    private Severity severity;

    private LoginResult(Severity severity) {
        this.severity = severity;
    }

    public Severity getSeverity() {
        return this.severity;
    }
}

==> Severity.java <==

public enum Severity {
    PASS,
    WAIVER,
    DENIAL;
}

==> Test.java <==

public class Test {

    public static void main(String[] args) {
        for (LoginResult r: LoginResult.values()){
            System.out.println(r + " " +r.getSeverity());           
        }
    }
}

Dane wyjściowe dla Test.java przedstawiające istotność dla każdego wyniku logowania:

NOT_LOGGED_IN : DENIAL
ALREADY_LOGGED_IN : PASS
INACTIVE_USER : DENIAL
VALID_USER : PASS
NEEDS_PASSWORD_CHANGE : WAIVER
INVALID_APP_USER : DENIAL
INVALID_NETWORK_USER : DENIAL
NON_APPROVED_ADDRESS : DENIAL
ACCOUNT_LOCKED : DENIAL
ACCOUNT_WILL_BE_LOCKED : WAIVER

Na podstawie zarówno wartości wyliczeniowej, jak i jej ważności, możesz zdecydować, czy tworzenie sesji będzie kontynuowane, czy nie.

EDYTOWAĆ:

W odpowiedzi na komentarz @ T.Sar zmieniłem możliwe wartości istotności na PASS, WAIVER i DENIAL zamiast (OK, WARNING and ERROR). W ten sposób jasne jest, że DENIAL (wcześniej ERROR) nie jest błędem per se i nie musi koniecznie przekładać się na zgłoszenie wyjątku. Program wywołujący sprawdza obiekt i decyduje, czy zgłosić wyjątek, ale DENIAL jest prawidłowym statusem wyniku wynikającym z wywołania processLogin(...).

• PASS: śmiało, utwórz sesję, jeśli jeszcze nie istnieje
• WAIVER: śmiało tym razem, ale następnym razem użytkownik może nie zostać dopuszczony do przejścia
• DENIAL: przepraszam, użytkownik nie może przejść, nie twórz sesji

Jest to przykład pierwotnej obsesji - używania pierwotnych typów do „prostych” zadań, które ostatecznie nie stają się takie proste.

Początkowo może to być kod, który boolzwraca a, aby wskazać powodzenie lub niepowodzenie, a następnie przekształcić w stan, w intktórym istniał trzeci stan, i ostatecznie stał się całą listą prawie nieudokumentowanych stanów błędów.

Typowym refaktoryzacją tego problemu jest utworzenie nowej klasy / struct / enum / object / cokolwiek, co może lepiej reprezentować daną wartość. W takim przypadku możesz rozważyć przejście na enumklasę zawierającą warunki wynikowe, a nawet klasę, która może zawierać komunikat boolo sukcesie lub niepowodzeniu, komunikat o błędzie, dodatkowe informacje itp.

Aby uzyskać więcej wzorów refaktoryzacji, które mogą być przydatne, zapoznaj się z kartą katalogową Zapachy do refaktoryzacji Industrial Logic .

Nazwałbym to przypadkiem „magicznych liczb” - liczb, które są wyjątkowe i same w sobie nie mają oczywistego znaczenia.

Refaktoryzacja, którą chciałbym tutaj zastosować, polega na przekształceniu typu zwracanego w wyliczenie, ponieważ zawiera on problem dotyczący domeny w typie. Radzenie sobie z wypadającymi błędami kompilacji powinno być możliwe fragmentarycznie, ponieważ wyliczenia Java można uporządkować i ponumerować. Nawet jeśli nie, nie powinno być trudno poradzić sobie z nimi bezpośrednio zamiast wracać do ints.

Jest to szczególnie nieprzyjemny fragment kodu. Antypattern jest znany jako „magiczne kody powrotu”. Można znaleźć dyskusję tutaj .

Wiele zwracanych wartości wskazuje na stany błędów. Istnieje ważna debata na temat tego, czy użyć obsługi błędów do kontroli przepływu, ale w twoim przypadku myślę, że istnieją 3 przypadki: sukces (kod 4), sukces, ale trzeba zmienić hasło (kod 5) i „niedozwolone”. Jeśli więc nie chcesz używać wyjątków do kontroli przepływu, możesz użyć wyjątków, aby wskazać te stany.

Innym podejściem byłoby przefakturowanie projektu, aby zwrócić obiekt „użytkownika” z atrybutem „profil” i „sesja” dla udanego logowania, atrybut „must_change_password”, jeśli to konieczne, oraz szereg atrybutów wskazujących, dlaczego dziennik - nie powiodło się, jeśli taki był przepływ.