¿Es un olor a código establecer una bandera en un bucle para usarla más tarde?

Tengo un código en el que itero un mapa hasta que cierta condición es verdadera y luego uso esa condición para hacer algunas cosas más.

Ejemplo:

Map<BigInteger, List<String>> map = handler.getMap();

if(map != null && !map.isEmpty())
{
    for (Map.Entry<BigInteger, List<String>> entry : map.entrySet())
    {
        fillUpList();

        if(list.size() > limit)
        {
            limitFlag = true;
            break;
        }
    }
}
else
{
    logger.info("\n>>>>> \n\t 6.1 NO entries to iterate over (for given FC and target) \n");
}

if(!limitFlag) // Continue only if limitFlag is not set
{
    // Do something
}

Siento establecer una bandera y luego usar eso para hacer más cosas es un olor a código.

Estoy en lo cierto? ¿Cómo podría eliminar esto?

No hay nada de malo en usar un valor booleano para su propósito: registrar una distinción binaria.

Si me pidieran que refactorizara este código, probablemente pondría el bucle en un método propio para que la asignación + se breakconvierta en a return; entonces ni siquiera necesitas una variable, simplemente puedes decir

if(fill_list_from_map()) {
  ...

No es necesariamente malo, y a veces es la mejor solución. Pero establecer banderas como esta en bloques anidados puede hacer que el código sea difícil de seguir.

El problema es que tiene bloques para delimitar ámbitos, pero luego tiene indicadores que se comunican entre ámbitos, rompiendo el aislamiento lógico de los bloques. Por ejemplo, el limitFlagserá falso si mapes null, por lo que el código "hacer algo" se ejecutará si mapes así null. Esto puede ser lo que pretende, pero podría ser un error que es fácil pasar por alto, porque las condiciones para este indicador se definen en otro lugar, dentro de un ámbito anidado. Si puede mantener la información y la lógica dentro del alcance más ajustado posible, debe intentar hacerlo.

Aconsejaría contra el razonamiento sobre 'olores de código'. Esa es la forma más perezosa posible de racionalizar sus propios prejuicios. Con el tiempo, desarrollarás muchos sesgos, y muchos de ellos serán razonables, pero muchos de ellos serán estúpidos.

En cambio, debe tener razones prácticas (es decir, no dogmáticas) para preferir una cosa sobre otra, y evitar pensar que debe tener la misma respuesta para todas las preguntas similares.

Los "olores de código" son para cuando no estás pensando. Si realmente va a pensar en el código, ¡hágalo bien!

En este caso, la decisión realmente podría ir en cualquier dirección dependiendo del código circundante. Realmente depende de lo que creas que es la forma más clara de pensar sobre lo que está haciendo el código. (el código "limpio" es un código que comunica claramente lo que está haciendo a otros desarrolladores y les facilita verificar que es correcto)

Muchas veces, las personas escriben métodos estructurados en fases, donde el código primero determinará lo que necesita saber sobre los datos y luego actuará sobre ellos. Si la parte "determinar" y la parte "actuar en consecuencia" son un poco complicadas, entonces puede tener sentido hacer esto, y a menudo el "lo que necesita saber" puede llevarse entre fases en las banderas booleanas. Sin embargo, realmente preferiría que le dieras un mejor nombre a la bandera. Algo así como "largeEntryExists" haría que el código fuera mucho más limpio.

Si, por otro lado, el código "// Do Something" es muy simple, entonces puede tener más sentido ponerlo dentro del ifbloque en lugar de establecer una bandera. Eso acerca el efecto a la causa, y el lector no tiene que escanear el resto del código para asegurarse de que la bandera conserve el valor que establecería.

Sí, es un olor a código (señales negativas de todos los que lo hacen).

La clave para mí es el uso de la breakdeclaración. Si no lo usó, estaría iterando sobre más elementos de los necesarios, pero usarlo le da dos posibles puntos de salida del bucle.

No es un problema importante con su ejemplo, pero puede imaginar que a medida que los condicionales o condicionales dentro del bucle se vuelven más complejos o el orden de la lista inicial se vuelve importante, es más fácil que un error se arrastre en el código.

Cuando el código es tan simple como su ejemplo, puede reducirse a un whilebucle o mapa equivalente, construir filtro.

Cuando el código es lo suficientemente complejo como para requerir marcas y saltos, será propenso a errores.

Así como con todos los olores de código: si ve una bandera, intente reemplazarla con un while. Si no puede, agregue pruebas unitarias adicionales.

Simplemente use un nombre que no sea limitFlag que indique lo que realmente está comprobando. ¿Y por qué registras algo cuando el mapa está ausente o vacío? limtFlag será falso, todo lo que te importa. El bucle está bien si el mapa está vacío, por lo que no es necesario verificarlo.

Establecer un valor booleano para transmitir información que ya tenía es una mala práctica en mi opinión. Si no hay una alternativa fácil, entonces probablemente sea indicativo de un problema mayor, como una pobre encapsulación.

Debe mover la lógica del bucle for al método fillUpList para que se rompa si se alcanza el límite. Luego verifique el tamaño de la lista directamente después.

Si eso rompe tu código, ¿por qué?

En primer lugar, el caso general: no es raro usar una bandera para verificar si algún elemento de una colección cumple una determinada condición. Pero el patrón que he visto con mayor frecuencia para resolver esto es mover el cheque en un método adicional y regresar directamente de él (como lo describió Kilian Foth en su respuesta ):

private <T> boolean checkCollection(Collection<T> collection)
{
    for (T element : collection)
        if (checkElement(element))
            return true;
    return false;
}

Desde Java 8 hay una manera más concisa usando Stream.anyMatch(…):

collection.stream().anyMatch(this::checkElement);

En su caso, esto probablemente se vería así (suponiendo list == entry.getValue()en su pregunta):

map.values().stream().anyMatch(list -> list.size() > limit);

El problema en su ejemplo específico es la llamada adicional a fillUpList(). La respuesta depende mucho de lo que se supone que debe hacer este método.

Nota al margen: tal como está, la llamada a fillUpList()no tiene mucho sentido, porque no depende del elemento que está iterando actualmente. Supongo que esto es una consecuencia de eliminar su código real para que se ajuste al formato de la pregunta. Pero exactamente eso lleva a un ejemplo artificial que es difícil de interpretar y, por lo tanto, difícil de razonar. Por lo tanto, es tan importante proporcionar un ejemplo mínimo, completo y verificable .

Así que supongo que el código real pasa la corriente entryal método.

Pero hay más preguntas que hacer:

• ¿Están vacías las listas en el mapa antes de llegar a este código? Si es así, ¿por qué ya hay un mapa y no solo la lista o el conjunto de BigIntegerclaves? Si no están vacíos, ¿por qué necesita completar las listas? Cuando ya hay elementos en la lista, ¿no es una actualización o algún otro cálculo en este caso?
• ¿Qué causa que una lista sea más grande que el límite? ¿Es esta una condición de error o se espera que ocurra con frecuencia? ¿Es causado por una entrada no válida?
• ¿Necesita las listas calculadas hasta el punto en que alcanza una lista más grande que el límite?
• ¿Qué hace la parte " Hacer algo "?
• ¿Reinicia el relleno después de esta parte?

Estas son solo algunas preguntas que me vinieron a la mente cuando intenté entender el fragmento de código. Entonces, en mi opinión, ese es el verdadero olor del código : su código no comunica claramente la intención.

Podría significar esto ("todo o nada" y alcanzar el límite indica un error):

/**
 * Computes the list of all foo strings for each passed number.
 * 
 * @param numbers the numbers to process. Must not be {@code null}.
 * @return all foo strings for each passed number. Never {@code null}.
 * @throws InvalidArgumentException if any number produces a list that is too long.
 */
public Map<BigInteger, List<String>> computeFoos(Set<BigInteger> numbers)
        throws InvalidArgumentException
{
    if (numbers.isEmpty())
    {
        // Do you actually need to log this here?
        // The caller might know better what to do in this case...
        logger.info("Nothing to compute");
    }
    return numbers.stream().collect(Collectors.toMap(
            number -> number,
            number -> computeListForNumber(number)));
}

private List<String> computeListForNumber(BigInteger number)
        throws InvalidArgumentException
{
    // compute the list and throw an exception if the limit is exceeded.
}

O podría significar esto ("actualizar hasta el primer problema"):

/**
 * Refreshes all foo lists after they have become invalid because of bar.
 * 
 * @param map the numbers with all their current values.
 *            The values in this map will be modified.
 *            Must not be {@code null}.
 * @throws InvalidArgumentException if any new foo list would become too long.
 *             Some other lists may have already been updated.
 */
public void updateFoos(Map<BigInteger, List<String>> map)
        throws InvalidArgumentException
{
    map.replaceAll(this::computeUpdatedList);
}

private List<String> computeUpdatedList(
        BigInteger number, List<String> currentValues)
        throws InvalidArgumentException
{
    // compute the new list and throw an exception if the limit is exceeded.
}

O esto ("actualice todas las listas pero mantenga la lista original si se vuelve demasiado grande"):

/**
 * Refreshes all foo lists after they have become invalid because of bar.
 * Lists that would become too large will not be updated.
 * 
 * @param map the numbers with all their current values.
 *            The values in this map will be modified.
 *            Must not be {@code null}.
 * @return {@code true} if all updates have been successful,
 *         {@code false} if one or more elements have been skipped
 *         because the foo list size limit has been reached.
 */
public boolean updateFoos(Map<BigInteger, List<String>> map)
{
    boolean allUpdatesSuccessful = true;
    for (Entry<BigInteger, List<String>> entry : map.entrySet())
    {
        List<String> newList = computeListForNumber(entry.getKey());
        if (newList.size() > limit)
            allUpdatesSuccessful = false;
        else
            entry.setValue(newList);
    }
    return allUpdatesSuccessful;
}

private List<String> computeListForNumber(BigInteger number)
{
    // compute the new list
}

O incluso lo siguiente (usando computeFoos(…)el primer ejemplo pero sin excepciones):

/**
 * Processes the passed numbers. An optimized algorithm will be used if any number
 * produces a foo list of a size that justifies the additional overhead.
 * 
 * @param numbers the numbers to process. Must not be {@code null}.
 */
public void process(Collection<BigInteger> numbers)
{
    Map<BigInteger, List<String>> map = computeFoos(numbers);
    if (isLimitReached(map))
        processLarge(map);
    else
        processSmall(map);
}

private boolean isLimitReached(Map<BigInteger, List<String>> map)
{
    return map.values().stream().anyMatch(list -> list.size() > limit);
}

O podría significar algo completamente diferente ... ;-)