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) { }
Odpowiedzi:
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):
==> LoginResult.java <==
==> Severity.java <==
==> Test.java <==
Dane wyjściowe dla Test.java przedstawiające istotność dla każdego wyniku logowania:
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(...)
.źródło
Severity
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
bool
zwraca a, aby wskazać powodzenie lub niepowodzenie, a następnie przekształcić w stan, wint
któ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
enum
klasę zawierającą warunki wynikowe, a nawet klasę, która może zawierać komunikatbool
o 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 .
źródło
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.
źródło
if (processLogin(..) == 3)
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.
źródło