Volver a trabajar una función que devuelve un código entero que representa muchos estados diferentes

Heredé un código horrible que incluí una pequeña muestra a continuación.

• ¿Hay un nombre para este antipatrón particular?

• ¿Cuáles son algunas recomendaciones para refactorizar esto?

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

El código es malo no solo por los números mágicos , sino porque fusiona varios significados en el código de retorno, ocultando dentro de su significado un error, una advertencia, un permiso para crear una sesión o una combinación de los tres, lo que lo convierte en un mal aporte para la toma de decisiones.

Sugeriría la siguiente refactorización: devolver una enumeración con los posibles resultados (como se sugiere en otras respuestas), pero agregar a la enumeración un atributo que indique si es una negación, una exención (lo dejaré pasar esta última vez) o si está bien (PASAR):

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());           
        }
    }
}

Salida para Test.java que muestra la gravedad de cada 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

Según el valor de enumeración y su gravedad, puede decidir si la creación de la sesión continúa o no.

EDITAR:

Como respuesta al comentario de @T.Sar, cambié los posibles valores de la gravedad a PASS, WAIVER y DENIAL en lugar de (OK, WARNING y ERROR). De esta manera, está claro que una DENEGACIÓN (anteriormente ERROR) no es un error per se y no necesariamente debe traducirse en arrojar una excepción. La persona que llama examina el objeto y decide si lanzar o no una excepción, pero DENIAL es un estado de resultado válido resultante de la llamada processLogin(...).

• PASS: adelante, crea una sesión si aún no existe
• RENUNCIA: adelante esta vez, pero la próxima vez que el usuario no se le permita pasar
• NEGACIÓN: lo siento, el usuario no puede pasar, no cree una sesión

Este es un ejemplo de obsesión primitiva : el uso de tipos primitivos para tareas "simples" que eventualmente no se vuelven tan simples.

Esto puede haber comenzado como un código que devolvió un boolpara indicar éxito o fracaso, luego se convirtió en un intcuando había un tercer estado y finalmente se convirtió en una lista completa de condiciones de error casi indocumentadas.

La refactorización típica para este problema es crear una nueva clase / estructura / enumeración / objeto / lo que sea que pueda representar mejor el valor en cuestión. En este caso, puede considerar cambiar a un enumque contenga las condiciones del resultado, o incluso una clase que pueda contener un boolmensaje de error, información adicional, etc.

Para obtener más patrones de refactorización que puedan ser útiles, eche un vistazo a la hoja de trucos de Industrial Logic Smells to Refactorings .

Yo llamaría a eso un caso de "números mágicos", números que son especiales y no tienen un significado obvio por sí mismos.

La refactorización que aplicaría aquí es reestructurar el tipo de retorno a una enumeración, ya que encapsula la preocupación del dominio en un tipo. Tratar con los errores de compilación que se caen debe ser posible poco a poco, ya que las enumeraciones de Java se pueden ordenar y numerar. Incluso si no, no debería ser difícil lidiar con ellos directamente en lugar de recurrir a los ints.

Este es un código de código particularmente desagradable. El antipatrón se conoce como "códigos de retorno mágicos". Puedes encontrar una discusión aquí .

Muchos de los valores de retorno indican estados de error. Existe un debate válido sobre si utilizar el manejo de errores para el control de flujo, pero en su caso, creo que hay 3 casos: éxito (código 4), éxito pero necesita cambiar la contraseña (código 5) y "no permitido". Por lo tanto, si no le importa usar excepciones para el control de flujo, puede usar excepciones para indicar esos estados.

Otro enfoque sería refactorizar el diseño para que devuelva un objeto "usuario", con un atributo "perfil" y "sesión" para un inicio de sesión exitoso, un atributo "must_change_password" si es necesario y un conjunto de atributos para indicar por qué el registro -in falló si ese era el flujo.