Überarbeitung einer Funktion, die einen ganzzahligen Code zurückgibt, der viele verschiedene Status darstellt

Ich habe einen schrecklichen Code geerbt, von dem ich unten ein kurzes Beispiel beigefügt habe.

• Gibt es einen Namen für dieses spezielle Anti-Muster?

• Was sind einige Empfehlungen für die Umgestaltung?

  // 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) { 
  }
  

Der Code ist nicht nur deshalb schlecht, weil die magischen Zahlen , sondern weil er mehrere Bedeutungen im Rückkehrcode zusammenführt und in seiner Bedeutung einen Fehler, eine Warnung, eine Erlaubnis zum Erstellen einer Sitzung oder eine Kombination der drei verbirgt, was ihn zu einer macht schlechte Eingabe für die Entscheidungsfindung.

Ich würde das folgende Refactoring vorschlagen: Rückgabe einer Aufzählung mit den möglichen Ergebnissen (wie in anderen Antworten vorgeschlagen), aber Hinzufügen eines Attributs zur Aufzählung, das angibt, ob es sich um eine Ablehnung, einen Verzicht (ich lasse Sie dies das letzte Mal bestehen lassen) oder wenn es OK ist (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());           
        }
    }
}

Ausgabe für Test.java mit dem Schweregrad für jedes LoginResult:

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

Anhand des Enum-Werts und seines Schweregrads können Sie entscheiden, ob die Erstellung der Sitzung fortgesetzt wird oder nicht.

BEARBEITEN:

Als Antwort auf den Kommentar von @ T.Sar habe ich die möglichen Werte des Schweregrads in PASS, WAIVER und DENIAL anstelle von (OK, WARNING und ERROR) geändert. Auf diese Weise ist klar, dass eine VERWEIGERUNG (früher FEHLER) an sich kein Fehler ist und nicht unbedingt eine Ausnahme auslösen muss . Der Aufrufer untersucht das Objekt und entscheidet, ob eine Ausnahme ausgelöst wird oder nicht. DENIAL ist jedoch ein gültiger Ergebnisstatus, der sich aus dem Aufruf ergibt processLogin(...).

• PASS: Erstellen Sie eine Sitzung, falls noch keine vorhanden ist
• Verzicht: Gehen Sie diesmal vor, aber beim nächsten Mal dürfen Benutzer möglicherweise nicht passieren
• VERWEIGERUNG: Entschuldigung, Benutzer kann nicht bestehen, keine Sitzung erstellen

Dies ist ein Beispiel für primitive Besessenheit - Verwendung primitiver Typen für "einfache" Aufgaben, die schließlich nicht mehr so ​​einfach werden.

Dies begann möglicherweise als Code, der a zurückgab bool, um Erfolg oder Misserfolg anzuzeigen, und dann in einen intZustand umgewandelt wurde, als es einen dritten Status gab, und schließlich zu einer vollständigen Liste von nahezu undokumentierten Fehlerzuständen wurde.

Das typische Refactoring für dieses Problem besteht darin, eine neue Klasse / struct / enum / object / zu erstellen, die den betreffenden Wert besser darstellen kann. In diesem Fall können Sie in Betracht ziehen, zu einer zu wechseln enum, die die Ergebnisbedingungen enthält, oder sogar zu einer Klasse, die eine boolfür Erfolg oder Misserfolg, eine Fehlermeldung, zusätzliche Informationen usw. enthält.

Weitere nützliche Refactoring-Muster finden Sie im Cheatsheet Smells to Refactorings von Industrial Logic .

Ich würde das einen Fall von "magischen Zahlen" nennen - Zahlen, die etwas Besonderes sind und für sich genommen keine offensichtliche Bedeutung haben.

Das Refactoring, das ich hier anwenden würde, besteht darin, den Rückgabetyp in eine Aufzählung umzustrukturieren, da es das Domänenproblem in einem Typ kapselt. Der Umgang mit den daraus resultierenden Kompilierungsfehlern sollte stückweise möglich sein, da Java-Enums bestellt und nummeriert werden können. Selbst wenn nicht, sollte es nicht schwierig sein, direkt mit ihnen umzugehen, anstatt auf Ints zurückzugreifen.

Dies ist ein besonders unangenehmer Code. Das Antimuster ist als "magische Rückkehrcodes" bekannt. Eine Diskussion finden Sie hier .

Viele der Rückgabewerte geben Fehlerzustände an. Es gibt eine gültige Debatte darüber, ob die Fehlerbehandlung für die Flusskontrolle verwendet werden soll, aber in Ihrem Fall gibt es meines Erachtens drei Fälle: Erfolg (Code 4), Erfolg, aber das Passwort muss geändert werden (Code 5) und "nicht erlaubt". Wenn Sie also keine Ausnahmen für die Flusssteuerung verwenden möchten, können Sie Ausnahmen verwenden, um diese Zustände anzuzeigen.

Ein anderer Ansatz wäre, das Design so umzugestalten, dass Sie ein "Benutzer" -Objekt mit einem "Profil" - und "Sitzungs" -Attribut für eine erfolgreiche Anmeldung, einem "must_change_password" -Attribut, falls erforderlich, und einer Reihe von Attributen zurückgeben, um anzugeben, warum das Protokoll erstellt wird -in fehlgeschlagen, wenn das der Fluss war.