Rilavorazione di una funzione che restituisce un codice intero che rappresenta molti stati diversi

Il codice è negativo non solo perché i numeri magici , ma perché combinano diversi significati nel codice di ritorno, nascondendo all'interno del suo significato un errore, un avvertimento, un permesso per creare una sessione o una combinazione dei tre, il che lo rende un input negativo per il processo decisionale.

Suggerirei il seguente refactoring: restituire un enum con i possibili risultati (come suggerito in altre risposte), ma aggiungendo all'enum un attributo che indica se si tratta di un rifiuto, una rinuncia (ti lascerò passare questo ultimo ora) o se è 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());           
        }
    }
}

Output per Test.java che mostra la gravità per ogni 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

In base al valore enumerato e alla sua gravità, puoi decidere se creare o meno i proventi della sessione.

Modifica

Come risposta al commento di @ T.Sar, ho modificato i possibili valori della severità su PASS, RIPOSIZIONE e DENIAL invece di (OK, AVVISO ed ERRORE). In questo modo è chiaro che un DENIAL (in precedenza ERRORE) non è un errore di per sé e non dovrebbe necessariamente tradurre in un'eccezione. Il chiamante esamina l'oggetto e decide se lanciare un'eccezione, ma DENIAL è uno stato di risultato valido risultante dal chiamare processLogin(...) .

• PASS: vai avanti, crea una sessione se non ne esiste già una
• RINUNCIA: vai avanti questa volta, ma l'utente della prossima volta potrebbe non essere autorizzato a passare
• DENIAL: mi dispiace, l'utente non può passare, non creare una sessione

Questo è un esempio di Obsession Primitive - utilizzando tipi primitivi per compiti "semplici" che alla fine non diventano così semplici.

Questo potrebbe essere iniziato come codice che ha restituito un bool per indicare il successo o l'insuccesso, quindi trasformato in int quando c'era un terzo stato, e alla fine è diventato un intero elenco di condizioni di errore quasi non documentate.

Il tipico refactoring per questo problema è creare una nuova classe / struct / enum / object / qualunque cosa possa rappresentare meglio il valore in questione. In questo caso, potresti prendere in considerazione il passaggio a enum che contiene le condizioni del risultato o anche una classe che potrebbe contenere bool per il successo o l'insuccesso, un messaggio di errore, informazioni aggiuntive, ecc.

Per ulteriori schemi di refactoring che potrebbero essere utili, dai uno sguardo al il cheatheet Smells to Refactorings .

Direi che si tratta di un caso di "numeri magici" - numeri che sono speciali e non hanno alcun significato ovvio da soli.

Il refactoring che vorrei applicare qui è di ristrutturare il tipo di ritorno in un enum, poiché incapsula il dominio in un tipo. Affrontare gli errori di compilazione che discendono da ciò dovrebbe essere possibile frammentario, dal momento che le enfasi java possono essere ordinate e numerate. Anche se non lo fosse, non dovrebbe essere difficile gestirli direttamente invece di ricorrere agli inert.

Questo è un bit di codice particolarmente sgradevole. L'antipattern è noto come "codici di ritorno magico". Puoi trovare una discussione qui .

Molti valori di ritorno indicano stati di errore. Esiste un dibattito valido sull'opportunità di utilizzare la gestione degli errori per il controllo del flusso, ma nel tuo caso, penso che ci siano 3 casi: il successo (codice 4), successo ma è necessario modificare la password (codice 5) e "non consentito". Quindi, se non ti interessa usare le eccezioni per il controllo del flusso, potresti usare le eccezioni per indicare quegli stati.

Un altro approccio sarebbe quello di rifattorizzare il progetto in modo da restituire un oggetto "utente", con un attributo "profilo" e "sessione" per un login riuscito, un attributo "must_change_password" se necessario e un mucchio di attributi per indicare perché il log-in è fallito se quello era il flusso.