Retravailler une fonction renvoyant un code entier qui représente de nombreux statuts différents

J'ai hérité d'un code horrible dont j'ai inclus un court échantillon ci-dessous.

• Y a-t-il un nom pour cet anti-modèle particulier?

• Quelles sont quelques recommandations pour refactoriser cela?

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

Le code est mauvais non seulement parce que les nombres magiques , mais parce qu'il fusionne plusieurs significations dans le code de retour, cachant à l'intérieur de sa signification une erreur, un avertissement, une autorisation de créer une session ou une combinaison des trois, ce qui en fait un mauvaise entrée pour la prise de décision.

Je suggérerais la refactorisation suivante: renvoyer une énumération avec les résultats possibles (comme suggéré dans d'autres réponses), mais en ajoutant à l'énumération un attribut indiquant s'il s'agit d'un déni, d'une renonciation (je vous laisse passer cette dernière fois) ou si c'est 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());           
        }
    }
}

Sortie pour Test.java montrant la gravité de chaque 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

En fonction de la valeur enum et de sa gravité, vous pouvez décider si la création de la session se poursuit ou non.

ÉDITER:

En réponse au commentaire de @ T.Sar, j'ai changé les valeurs possibles de la gravité en PASS, WAIVER et DENIAL au lieu de (OK, WARNING et ERROR). De cette façon, il est clair qu'un REFUS (auparavant ERREUR) n'est pas une erreur en soi et ne devrait pas nécessairement se traduire par le lancement d'une exception. L'appelant examine l'objet et décide de lever ou non une exception, mais DENIAL est un état de résultat valide résultant de l'appel processLogin(...).

• PASS: allez-y, créez une session si elle n'existe pas déjà
• RENONCIATION: allez-y cette fois, mais la prochaine fois, vous ne serez peut-être pas autorisé à passer
• DENIAL: désolé, l'utilisateur ne peut pas passer, ne créez pas de session

Ceci est un exemple d' obsession primitive - en utilisant des types primitifs pour des tâches "simples" qui finissent par devenir moins simples.

Cela peut avoir commencé comme un code qui renvoyait un boolpour indiquer le succès ou l'échec, puis s'est transformé en un intquand il y avait un troisième état, et est finalement devenu une liste complète de conditions d'erreur presque non documentées.

La refactorisation typique de ce problème consiste à créer une nouvelle classe / struct / enum / objet / tout ce qui peut mieux représenter la valeur en question. Dans ce cas, vous pouvez envisager de basculer vers une enumqui contient les conditions de résultat, ou même une classe qui pourrait contenir une boolréussite ou un échec, un message d'erreur, des informations supplémentaires, etc.

Pour plus de modèles de refactorisation qui pourraient être utiles, jetez un coup d'œil à la feuille de triche Smells to Refactorings d' Industrial Logic .

J'appellerais cela un cas de "nombres magiques" - des nombres qui sont spéciaux et n'ont pas de signification évidente en eux-mêmes.

Le refactoring que j'appliquerais ici est de restructurer le type de retour en une énumération, car il encapsule le domaine concerné dans un type. Traiter les erreurs de compilation qui en découlent devrait être possible au coup par coup, car les énumérations java peuvent être ordonnées et numérotées. Même si ce n'est pas le cas, il ne devrait pas être difficile de les traiter directement au lieu de retomber dans les mœurs.

C'est un morceau de code particulièrement désagréable. L'antipattern est connu sous le nom de "codes de retour magiques". Vous pouvez trouver une discussion ici .

De nombreuses valeurs de retour indiquent des états d'erreur. Il y a un débat valide sur l'opportunité d'utiliser la gestion des erreurs pour le contrôle de flux, mais dans votre cas, je pense qu'il y a 3 cas: succès (code 4), succès mais besoin de changer de mot de passe (code 5) et "non autorisé". Donc, si vous ne vous souciez pas de l'utilisation d'exceptions pour le contrôle de flux, vous pouvez utiliser des exceptions pour indiquer ces états.

Une autre approche serait de refactoriser la conception afin que vous renvoyiez un objet "utilisateur", avec un attribut "profil" et "session" pour une connexion réussie, un attribut "must_change_password" si nécessaire, et un tas d'attributs pour indiquer pourquoi le journal -in a échoué si tel était le flux.