¿Es este uso de condicionales un antipatrón?

Lo he visto mucho en nuestro sistema heredado en el trabajo, 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, en el camino, 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 que realmente me he tomado el tiempo para entender, podrían ser refactorizados para evitar usar la bandera. ¿Pero es este un antipatró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 sobre si alguna vez es necesario (en un lenguaje OO moderno) crear una variable de bandera local, posiblemente configurándola en varios lugares, y luego usarla más tarde para decidir si ejecutar el siguiente bloque de código.

No sé sobre antipatrón, pero extraería tres métodos de esto.

El primero realizaría algún 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 llamar solo al segundo (y siempre) si el primer método devuelve verdadero.

Al nombrar bien los métodos, espero que aclare el código.

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 hay un debate sobre si los retornos tempranos son aceptables, pero ese es un detalle de implementación (como 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 cual es bueno ...

Uno de los comentarios sobre la pregunta sugiere que este patrón representa un olor , y estaría de acuerdo con eso. Vale la pena mirarlo para ver si puede aclarar la intención.

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 olores de código por derecho propio; los antipatrones son cosas más abstractas y complejas de la OMI).

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

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

O incluso puede deshacerse por completo de la bandera local ocultando la marca de verificación en el segundo método y utilizando un formulario como

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

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

todo es un mal nombre 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 aún más legible:

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

El patrón de la máquina de estado me parece bien. Los antipatrones allí son "todo" (nombre incorrecto) y "mucho código".

Depende realmente. Si el código guardado por todo(¡espero que no esté usando ese nombre de verdad, ya que no es totalmente mnemotécnico!) Es un código de limpieza conceptual, entonces tiene un antipatrón y debería usar algo como RAII de C ++ o C # 's usingconstruir para manejar las cosas en su lugar.

Por otro lado, si conceptualmente no es una etapa de limpieza, sino más bien un procesamiento adicional que a veces es necesario y donde la decisión de hacerlo debe tomarse antes, lo que está escrito está bien. Considere si los fragmentos de código individuales se refactorizarían mejor 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á sucediendo sea menos claro, y eso definitivamente sería un antipatrón.

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

Creo que esta es la parte "antipatrón" de lo que estás viendo. Encuentre una herramienta que mida la complejidad ciclomática de su código: existen complementos para eclipse. Es esencialmente una medida de cuán difícil es probar su código e involucra el número y los niveles de las ramas del 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 tareas: cada tarea es propia objeto y en mitad de la tarea tiene la capacidad de poner en cola la siguiente tarea instanciando otro objeto de tarea y arrojándolo a la cola. Estos pueden ser increíblemente simples de configurar y reducen significativamente la complejidad de ciertos tipos de código, pero como dije, esta es una puñalada total en la oscuridad.

Usando el ejemplo anterior de pdr, como es un buen ejemplo, iré un paso más allá.

Él tuvo:

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 ordenar?

Verá que su método BuildList parece saber sobre la ordenación, ya que devuelve un bool que lo indica como tal, pero eso no tiene sentido para un método diseñado para crear una lista.

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

También modelos de dominio ricos, en ese caso, solo un revestimiento hará grandes cosas dentro del objeto.

Si la bandera solo se establece una vez, mueva el
...
código directamente después de
... // algún otro código aquí y
luego elimine la bandera.

De lo contrario, divida los
... // lotes de 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 que es la lógica de ramificación.

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

El código en sí no es el antipatrón que está sucediendo aquí ... Sospecho que mezclar la lógica de bifurcación con la ejecución real de cosas (comandos) es el antipatrón que estás buscando.

A veces encuentro que necesito implementar este patrón. En ocasiones, desea realizar varias verificaciones antes de continuar con una operación. Por razones de eficiencia, los cálculos que involucran ciertas verificaciones no se realizan a menos que parezca absolutamente necesario verificar. Por lo tanto, normalmente ve 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 realización de la operación, para que vea 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 de acuerdo con lo que está tratando de 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 los métodos internos que devuelven indicaciones booleanas de éxito o fracaso que simplifican aún más las cosas, aunque a algunos programadores les gustan los métodos extremadamente largos por alguna extraña razón.

Este 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, su ejemplo específico puede reescribirse 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 omitió. Concéntrese en hacer que ese código sea más conciso.

Las banderas locales utilizadas para el flujo de control deben reconocerse como una forma de gotodisfraz. Si un indicador solo se usa dentro de una función, podría eliminarse escribiendo dos copias de la función, etiquetando uno como "indicador es verdadero" y el otro como "indicador es falso", y reemplazando cada operación que establece el indicador 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 el uso de un indicador será más limpio que cualquier alternativa posible que use en su gotolugar, pero eso no siempre es cierto. En algunos casos, usar gotopara omitir un fragmento de código puede ser más limpio que usar banderas para hacerlo [aunque algunas personas podrían insertar una caricatura de rapaces aquí].

Creo que el principio rector principal debería ser que el flujo de la lógica del programa debe parecerse a la descripción de la lógica de negocios en la medida de lo posible. Si los requisitos de lógica de negocios se describen en términos de estados que se dividen y fusionan de manera extraña, hacer que la lógica del programa haga lo mismo puede ser más limpio que tratar de usar banderas para ocultar dicha lógica. Por otro lado, si la forma más natural de describir las reglas de negocio sería decir que una acción se debe hacer si se han realizado ciertas otras acciones, la forma más natural de expresar eso puede ser usar una bandera que se establece al realizar las últimas acciones y por lo demás están claras.