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

10

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) { 
    }
    
A_B
fuente
2
¿Qué son "found-estably" y "needed-estably" ?
Tulains Córdova
44
Se supone que es un guión em , que se lee como "usuario válido encontrado: establezca su sesión".
BJ Myers
2
@A_B ¿Cuáles de esos valores de retorno son inicios de sesión exitosos? ¿Cuáles son los inicios de sesión fallidos? No todos son evidentes.
Tulains Córdova
@A_B ¿"establecer su sesión" significa "sesión establecida" o "necesita establecer una sesión"?
Tulains Córdova
@ TulainsCórdova: "Establecer" significa tanto como "crear" (al menos en este contexto), por lo que "establecer su sesión" es aproximadamente igual a "crear su sesión"
hoffmale

Respuestas:

22

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
Tulains Córdova
fuente
También puede crear una enumeración "compleja" (enumeración con atributos) para incrustar el nivel de error en la enumeración. Sin embargo, tenga cuidado porque si usa algunas herramientas de serialización, eso podría no pasar muy bien.
Walfrat
Lanzar excepciones en los casos de error y guardar las enumeraciones solo para el éxito también es una opción.
T. Sar
@ T.Sar Bueno, como entendí, no son errores per se sino negaciones para crear una sesión por alguna razón. Editaré la respuesta.
Tulains Córdova
@ T.Sar Cambié los valores a PASS, WAIVER y DENIAL para dejar en claro que lo que anteriormente llamé ERROR es un estado válido. Tal vez ahora debería encontrar un mejor nombre paraSeverity
Tulains Córdova
Estaba pensando en otra cosa con mi sugerencia, ¡pero realmente me gustó tu sugerencia! Estoy arrojando un +1, ¡seguro!
T. Sar
15

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 .

BJ Myers
fuente
7

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.

Daenyth
fuente
Eso no es lo que generalmente se entiende con 'números mágicos'.
D Drmmr
2
Se mostrará como números mágicos en los sitios de llamadas, como enif (processLogin(..) == 3)
Daenyth
@DDrmmr: esto es exactamente lo que significa el olor del código de 'números mágicos'. Esta firma de función casi ciertamente implica que processLogin () contiene líneas como "return 8;" en su implementación, y casi obliga al código que usa processLogin () a parecerse a esto "if (resultFromProcessLogin == 7) {".
Stephen C. Steel
3
@Stephen El valor real de los números es irrelevante aquí. Son solo identificaciones. El término números mágicos generalmente se usa para valores en el código que tienen un significado, pero el significado de quién no está documentado (por ejemplo, en un nombre de variable). Reemplazar los valores aquí con variables enteras con nombre no solucionará el problema.
D Drmmr
2

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.

Neville Kuyt
fuente