¿Este uso de condicionales es un antipatrón?

14

Esto lo he visto mucho en nuestro sistema heredado en funcionamiento: funciones que se parecen a esto:

bool todo = false;
if(cond1)
{
  ... // lots of code here
  if(cond2)
    todo = true;
  ... // some other code here
}

if(todo)
{
  ...
}

En otras palabras, la función tiene dos partes. La primera parte realiza algún tipo de procesamiento (que potencialmente contiene bucles, efectos secundarios, etc.) y, a lo largo del proceso, puede establecer el indicador "todo". La segunda parte solo se ejecuta si se ha establecido el indicador "todo".

Parece una forma bastante fea de hacer las cosas, y creo que la mayoría de los casos en los que me he tomado el tiempo de entender, podrían ser refaccionados para evitar el uso de la bandera. ¿Pero es este un anti-patrón real, una mala idea o perfectamente aceptable?

La primera refactorización obvia sería cortarla en dos métodos. Sin embargo, mi pregunta es más acerca de si alguna vez existe la necesidad (en un lenguaje moderno de OO) de crear una variable de marca local, configurarlo potencialmente en múltiples lugares y luego usarla para decidir si ejecutar el siguiente bloque de código.

    
pregunta Kricket 11.10.2011 - 10:42
fuente

12 respuestas

23

No sé sobre el anti-patrón, pero extraería tres métodos de esto.

El primero realizaría un trabajo y devolvería un valor booleano.

El segundo realizaría cualquier trabajo realizado por "algún otro código"

El tercero realizaría el trabajo auxiliar si el valor booleano devuelto fuera verdadero.

Los métodos extraídos probablemente serían privados si fuera importante que solo se llame al segundo (y siempre) si el primer método se vuelve verdadero.

Al nombrar bien los métodos, espero que el código sea más claro.

Algo como esto:

public void originalMethod() {
    bool furtherProcessingRequired = lotsOfCode();
    someOtherCode();
    if (furtherProcessingRequired) {
        doFurtherProcessing();
    }
    return;
}

private boolean lotsOfCode() {
    if (cond1) {
        ... // lots of code here
        if(cond2) {
            return true;
        }
    }
    return false;
}

private void someOtherCode() {
    ... // some other code here
}

private void doFurtherProcessing() {
    // Do whatever is needed
}

Obviamente, se debe debatir si los retornos iniciales son aceptables, pero eso es un detalle de la implementación (como lo es el estándar de formato de código).

El punto es que la intención del código se vuelve más clara, lo que es bueno ...

Uno de los comentarios sobre la pregunta sugiere que este patrón representa un smell , y estoy de acuerdo con eso. Vale la pena verlo para ver si puede aclarar la intención.

    
respondido por el Bill Michell 11.10.2011 - 10:48
fuente
6

Creo que la fealdad se debe al hecho de que hay mucho código en un solo método, y / o las variables están mal nombradas (las cuales son code smells por derecho propio. Los antipatterns son cosas más abstractas y complejas (IMO).

Entonces, si extraes la mayoría del código en métodos de nivel inferior como sugiere @Bill, el resto se vuelve limpio (al menos para mí). Por ejemplo,

bool registrationNeeded = installSoftware(...);
if (registrationNeeded) {
  registerUser(...)
}

O incluso puede deshacerse completamente de la bandera local ocultando el control de bandera en el segundo método y utilizando un formulario como

calculateTaxRefund(isTaxRefundable(...), ...)

En general, no veo que una variable de marca local sea necesariamente mala per se. La opción de la anterior es más legible (= preferible para mí) depende de la cantidad de parámetros del método, los nombres elegidos y la forma más consistente con la lógica interna del código.

    
respondido por el Péter Török 11.10.2011 - 11:19
fuente
4

todo es un nombre realmente malo para la variable, pero creo que eso podría ser todo lo que está mal. Es difícil estar completamente seguro sin el contexto.

Digamos que la segunda parte de la función ordena una lista, construida por la primera parte. Esto debería ser mucho más legible:

bool requiresSorting = false;
if(cond1)
{
    ... // lots of code here
    if(cond2)
        requiresSorting = true;
    ... // some other code here
}

if(requiresSorting)
{
    ...
}

Sin embargo, la sugerencia de Bill también es correcta. Esto es más legible aún:

bool requiresSorting = BuildList(list);
if (requiresSorting)
    SortList(list);
    
respondido por el pdr 11.10.2011 - 11:18
fuente
2

El patrón de la máquina de estado me parece bien. Los patrones anti están "todo" (nombre incorrecto) y "muchos códigos".

    
respondido por el ptyx 11.10.2011 - 20:00
fuente
1

Depende realmente. Si el código protegido por todo (¡espero que no estés usando ese nombre de verdad, ya que es totalmente no mnemotécnico!) Es un código de limpieza conceptual, entonces tienes un anti-patrón y deberías usar algo como el de C ++ RAII o la construcción using de C # para manejar las cosas en su lugar.

Por otra parte, si conceptualmente no es una etapa de limpieza sino un proceso adicional que a veces se necesita y donde la decisión de hacerlo debe tomarse antes, lo que está escrito está bien . Considere si los fragmentos de código individuales serían mejor refaccionados en sus propias funciones, por supuesto, y también si ha capturado el significado de la variable de bandera en su nombre, pero este patrón de código básico está bien. En particular, tratar de poner demasiado en otras funciones podría hacer que lo que está pasando sea menos claro, y que definitivamente sería un antipatrón.

    
respondido por el Donal Fellows 11.10.2011 - 11:23
fuente
1

Muchas de las respuestas aquí tendrían problemas para pasar una verificación de complejidad, algunas miradas > 10.

Creo que esta es la parte "anti-patrón" de lo que estás viendo. Encuentre una herramienta que mida la complejidad ciclomática de su código: hay complementos para eclipse. Esencialmente, es una medida de cuán difícil es probar su código e involucra el número y los niveles de las ramas de código.

Como una suposición total de una posible solución, el diseño de su código me hace pensar en "Tareas", si esto sucede en muchos lugares, tal vez lo que realmente desea es una arquitectura orientada a la tarea, cada tarea siendo su propio objeto y en medio de la tarea, tiene la capacidad de poner en cola la siguiente tarea al crear una instancia de otro objeto de tarea y lanzarlo a la cola. Estos pueden ser increíblemente sencillos de configurar y reducen significativamente la complejidad de ciertos tipos de código, pero como dije, esto es una puñalada total en la oscuridad.

    
respondido por el Bill K 11.10.2011 - 17:43
fuente
1

Utilizando el ejemplo anterior de pdr, ya que es un buen ejemplo, iré un paso más allá.

Él tenía:

bool requiresSorting = BuildList(list);
if (requiresSorting)
    SortList(list);

Entonces me di cuenta de que lo siguiente funcionaría:

if(BuildList(list)) 
    SortList(list)

Pero no es tan claro.

Entonces, a la pregunta original, ¿por qué no tener:

BuildList(list)
SortList(list)

¿Y dejar que SortList decida si requiere ordenación?

Usted ve que su método BuildList parece saber acerca de la clasificación, ya que devuelve un bool que indica como tal, pero eso no tiene sentido para un método que está diseñado para construir una lista.

    
respondido por el Ian 11.10.2011 - 13:25
fuente
0

Sí, esto parece ser un problema porque tiene que seguir rastreando todos los lugares en los que está marcando el indicador de ON / OFF. Es mejor incluir la lógica dentro de una condición anidada if en lugar de eliminar la lógica.

También modelos de dominio enriquecidos, en ese caso, solo un trazador de líneas hará grandes cosas dentro del objeto.

    
respondido por el java_mouse 11.10.2011 - 15:45
fuente
0

Si el indicador solo se establece una vez, mueva el botón. ...
código hasta directamente después del
... // algún otro código aquí
luego acabar con la bandera.

De lo contrario, divide el
  ... // mucho código aquí
... // algún otro código aquí
...
Codifique en funciones separadas si es posible, por lo que está claro que esta función tiene una responsabilidad: la lógica de bifurcación.

Siempre que sea posible, separe el código dentro de
  ... // mucho código aquí
en dos o más funciones, algunas que hacen algo de trabajo (que es un comando) y otras que devuelven el valor de la tarea (que es una consulta) o lo hacen muy explícito, lo están modificando (una consulta que usa efectos secundarios)

El código en sí mismo no es el anti-patrón que está ocurriendo aquí ... Sospecho que la combinación de lógica de bifurcación con la ejecución real de cosas (comandos) es el anti-patrón que estás buscando.

    
respondido por el andrew pate 08.06.2018 - 17:21
fuente
0

A veces encuentro que necesito implementar este patrón. A veces desea realizar varias comprobaciones antes de continuar con una operación. Por razones de eficiencia, los cálculos que involucran ciertas comprobaciones no se realizan a menos que parezca absolutamente necesario verificar. Así que normalmente ves un código como:

// Check individual fields for proper input

if(fieldsValidated) {
  // Perform cross-checks to see if input contains values which exist in the database

  if(valuesExist) {
    try {
      // Attempt insertion
      trx.commit();
    } catch (DatabaseException dbe) {
      trx.rollback();
      throw dbe;
    }
  } else {
    closeConnection(db);
    throwException();
  }
} else {
  closeConnection(db);
  throwException();
}

Esto podría simplificarse separando la validación del proceso real de realizar la operación, por lo que vería más como:

boolean proceed = true;
// Check individual fields for proper input

if(fieldsValidated) {
  // Perform cross-checks to see if input contains values which exist in the database

  if(!valuesExist) {
    proceed = false;
  }
} else {
  proceed = false;
}

// The moment of truth
if(proceed) {
  try {
    // Attempt insertion
    trx.commit();
  } catch (DatabaseException dbe) {
    trx.rollback();
    throw dbe;
  }
} else {
  if(db.isOpen()) {
    closeConnection(db);
  }
  throwException();
}

Obviamente, varía según lo que intenta lograr, aunque escrito de esta manera, tanto su código de "éxito" como su código de "falla" se escriben una vez, lo que simplifica la lógica y mantiene el mismo nivel de rendimiento. A partir de ahí, sería una buena idea ajustar niveles completos de validación dentro de métodos internos que devuelven indicaciones booleanas de éxito o fracaso que simplifiquen aún más las cosas, aunque a algunos programadores les gustan los métodos extremadamente largos por alguna extraña razón.

    
respondido por el Neil 11.10.2011 - 14:39
fuente
0

Esto no es un patrón . La interpretación más general es que está configurando una variable booleana y ramificando su valor más adelante. Esa es la programación procesal normal, nada más.

Ahora, tu ejemplo específico se puede reescribir como:

if(cond1)
{
    ... // lots of code here
    ... // some other code here
    if (cond2)
    {
        ...
    }
}

Eso puede ser más fácil de leer. O tal vez no. Depende del resto del código que haya omitido. Enfócate en hacer que ese código sea más conciso.

    
respondido por el D Drmmr 13.06.2018 - 21:11
fuente
-1

Las banderas locales utilizadas para el flujo de control deben reconocerse como una forma de goto disfrazada. Si solo se usa una bandera dentro de una función, podría eliminarse escribiendo dos copias de la función, etiquetando una como "bandera es verdadera" y la otra como "bandera es falsa", y reemplazando cada operación que establece la bandera cuando está claro, o lo borra cuando está configurado, con un salto entre las dos versiones de la función.

En muchos casos, el código que usa una marca será más limpio que cualquier alternativa posible que use goto en su lugar, pero eso no siempre es cierto. En algunos casos, usar goto para omitir una parte del código puede ser más limpio que usar banderas para hacerlo [aunque algunas personas podrían insertar una caricatura de rapaz aquí].

Creo que el principio guía principal debe ser que el flujo lógico del programa debe parecerse a la descripción de la lógica empresarial en la medida de lo posible. Si los requisitos de la lógica de negocios se describen en términos de estados que se dividen y combinan de formas extrañas, hacer que la lógica del programa funcione de manera similar puede ser más limpio que tratar de usar indicadores para ocultar dicha lógica. Por otro lado, si la forma más natural de describir las reglas de negocios sería decir que se debe realizar una acción si se hubieran realizado otras acciones, la forma más natural de expresar eso sería utilizar una bandera que se establece al realizar las últimas acciones y es por lo demás claro.

    
respondido por el supercat 08.06.2018 - 20:19
fuente

Lea otras preguntas en las etiquetas