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

10

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) { 
    }
    
A_B
źródło
2
Co to jest „założony-ustalony” i „potrzebny-ustalony” ?
Tulains Córdova
4
To ma być myślnik , czytany jak „znaleziono prawidłowego użytkownika: ustanów sesję”.
BJ Myers
2
@A_B Które z tych zwracanych wartości są udanymi logowaniami, a które nieudanymi logowaniami. Nie wszystkie są oczywiste.
Tulains Córdova
@A_B Czy „ustanowienie sesji” oznacza „ustanowienie sesji” czy „wymaga ustanowienia sesji”?
Tulains Córdova
@ TulainsCórdova: „Ustanowienie” oznacza tyle, co „utwórz” (przynajmniej w tym kontekście) - więc „Ustanowienie ich sesji” jest mniej więcej równe „Ustanowienie ich sesji”
hoffmale

Odpowiedzi:

22

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
Tulains Córdova
źródło
możesz także zbudować „złożony” wyliczenie (wyliczenie z atrybutami), aby osadzić poziom błędu w wyliczeniu. Bądź jednak ostrożny, ponieważ jeśli używasz narzędzi do serializacji somme, może to nie przebiegać zbyt dobrze.
Walfrat
Zgłaszanie wyjątków w przypadkach błędów i oszczędzanie wyliczeń tylko dla sukcesu jest również opcją.
T. Sar
@ T.Sar Cóż, jak zrozumiałem, nie są to błędy same w sobie, ale z jakiegoś powodu odmowa utworzenia sesji. Zmienię odpowiedź.
Tulains Córdova
@ T.Sar Zmieniłem wartości na PASS, WAIVER i DENIAL, aby wyjaśnić, że to, co wcześniej nazywałem ERROR, jest poprawnym statusem. Może teraz powinienem wymyślić lepszą nazwę dlaSeverity
Tulains Córdova
Myślałem o czymś innym z moją sugestią, ale naprawdę podobała mi się twoja sugestia! Na pewno wyrzucam +1!
T. Sar
15

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 .

BJ Myers
źródło
7

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.

Daenyth
źródło
Nie to zwykle oznacza „magiczne liczby”.
D Drmmr
2
Pojawi się jako magiczne numery na stronach wywoławczych, jak wif (processLogin(..) == 3)
Daenyth
@DDrmmr - właśnie to rozumie zapach kodu „magicznych liczb”. Ta sygnatura funkcji prawie na pewno sugeruje, że processLogin () zawiera wiersze takie jak „return 8;” w swojej implementacji, i prawie zmusza kod za pomocą processLogin (), aby wyglądał mniej więcej tak: „if (resultFromProcessLogin == 7) {”.
Stephen C. Steel
3
@Stephen Rzeczywista wartość liczb nie ma tutaj znaczenia. To tylko dowody osobiste. Termin magiczne liczby jest zwykle używany do wartości w kodzie, które mają znaczenie, ale kto nie jest udokumentowany (np. W nazwie zmiennej). Zastąpienie tutaj wartości nazwanymi zmiennymi liczbami całkowitymi nie rozwiąże problemu.
D Drmmr
2

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.

Neville Kuyt
źródło