É um cheiro de código definir um sinalizador em um loop para usá-lo mais tarde?

Eu tenho um pedaço de código no qual itero um mapa até que uma determinada condição seja verdadeira e, posteriormente, uso essa condição para fazer mais algumas coisas.

Exemplo:

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
}

Sinto que definir uma bandeira e depois usá-la para fazer mais coisas é um cheiro de código.

Estou certo? Como eu poderia remover isso?

Não há nada de errado em usar um valor booleano para a finalidade a que se destina: registrar uma distinção binária.

Se me dissessem para refatorar esse código, provavelmente colocaria o loop em um método próprio para que a atribuição + se breaktransforma em a return; então você nem precisa de uma variável, você pode simplesmente dizer

if(fill_list_from_map()) {
  ...

Não é necessariamente ruim e, às vezes, é a melhor solução. Mas definir sinalizadores como este em blocos aninhados pode dificultar o código.

O problema é que você tem blocos para delimitar escopos, mas depois possui sinalizadores que se comunicam entre os escopos, quebrando o isolamento lógico dos blocos. Por exemplo, o valor limitFlagserá falso se mapfor null, portanto o código "faça alguma coisa" será executado se mapfor null. Pode ser o que você pretende, mas pode ser um erro fácil de perder, porque as condições para esse sinalizador são definidas em outro lugar, dentro de um escopo aninhado. Se você conseguir manter as informações e a lógica dentro do escopo mais rígido possível, tente fazê-lo.

Eu desaconselharia o raciocínio sobre 'odores de código'. Essa é apenas a maneira mais preguiçosa possível de racionalizar seus próprios preconceitos. Com o tempo, você desenvolverá muitos preconceitos, e muitos deles serão razoáveis, mas muitos serão estúpidos.

Em vez disso, você deve ter razões práticas (ou seja, não dogmáticas) para preferir uma coisa a outra e evitar pensar que deve ter a mesma resposta para todas as perguntas semelhantes.

"Código cheira" é para quando você não está pensando. Se você realmente pensa no código, faça o que é certo!

Nesse caso, a decisão poderia realmente ser de qualquer maneira, dependendo do código ao redor. Realmente depende do que você acha que é a maneira mais clara de pensar sobre o que o código está fazendo. (código "limpo" é aquele que comunica claramente o que está fazendo com outros desenvolvedores e facilita a verificação de que está correto)

Muitas vezes, as pessoas escrevem métodos estruturados em fases, nas quais o código determina primeiro o que ele precisa saber sobre os dados e depois age sobre eles. Se a parte "determinar" e a parte "agir sobre ela" são um pouco complicadas, pode fazer sentido fazer isso, e muitas vezes o "o que ele precisa saber" pode ser carregado entre fases nos sinalizadores booleanos. Eu realmente preferiria que você desse um nome melhor à bandeira. Algo como "largeEntryExists" tornaria o código muito mais limpo.

Se, por outro lado, o código "// Do Something" for muito simples, poderá fazer mais sentido colocá-lo dentro do ifbloco em vez de definir um sinalizador. Isso aproxima o efeito da causa, e o leitor não precisa varrer o restante do código para garantir que o sinalizador retenha o valor que você definiria.

Sim, é um cheiro de código (sugestão reduzida de todos que o fazem).

O principal para mim é o uso da breakdeclaração. Se você não o utilizasse, estaria iterando sobre mais itens do que o necessário, mas usá-lo fornece dois pontos de saída possíveis do loop.

Não é um problema importante no seu exemplo, mas você pode imaginar que, à medida que as condicionais ou condicionais dentro do loop se tornam mais complexas ou a ordem da lista inicial se torna importante, é mais fácil para um bug entrar no código.

Quando o código é tão simples quanto o seu exemplo, ele pode ser reduzido a um whileloop ou mapa equivalente, construção de filtro.

Quando o código é complexo o suficiente para exigir sinalizadores e quebras, ele estará sujeito a erros.

Assim como todos os odores de código: Se você vir um sinalizador, tente substituí-lo por um while. Se não conseguir, adicione testes de unidade extras.

Basta usar um nome diferente de limitFlag que informe o que você está realmente verificando. E por que você registra alguma coisa quando o mapa está ausente ou vazio? limtFlag será falso, tudo o que importa. O loop é bom se o mapa estiver vazio, portanto, não é necessário verificar isso.

Definir um valor booleano para transmitir informações que você já possui é uma prática ruim na minha opinião. Se não houver uma alternativa fácil, provavelmente é indicativo de um problema maior, como um encapsulamento ruim.

Você deve mover a lógica do loop for para o método fillUpList para quebrá-lo se o limite for atingido. Em seguida, verifique o tamanho da lista diretamente depois.

Se isso quebra seu código, por quê?

Primeiro do caso geral: não é incomum usar um sinalizador para verificar se algum elemento de uma coleção atende a uma determinada condição. Mas o padrão que vi com mais freqüência para resolver isso é mover a verificação em um método extra e retornar diretamente a partir dela (como Kilian Foth descrito em sua resposta ):

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

Desde o Java 8, existe uma maneira mais concisa de usar Stream.anyMatch(…):

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

No seu caso, isso provavelmente seria assim (supondo list == entry.getValue()na sua pergunta):

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

O problema no seu exemplo específico é a chamada adicional para fillUpList(). A resposta depende muito do que esse método deve fazer.

Nota lateral: do jeito que está, a chamada para fillUpList()não faz muito sentido, porque não depende do elemento que você está iterando no momento. Eu acho que isso é uma consequência de remover o código real para se ajustar ao formato da pergunta. Mas exatamente isso leva a um exemplo artificial difícil de interpretar e, portanto, difícil de raciocinar. Por isso, é tão importante para fornecer um mínimo, completa e verificável exemplo .

Então, eu assumo que o código real passa a corrente entrypara o método

Mas há mais perguntas a serem feitas:

• As listas no mapa estão vazias antes de alcançar este código? Se sim, por que já existe um mapa e não apenas a lista ou o conjunto de BigIntegerchaves? Se eles não estiverem vazios, por que você precisa preencher as listas? Quando já existem elementos na lista, não é uma atualização ou alguma outra computação nesse caso?
• O que faz com que uma lista fique maior que o limite? Esta é uma condição de erro ou espera-se que ocorra com frequência? É causado por entrada inválida?
• Você precisa das listas calculadas até o ponto em que atinge uma lista maior que o limite?
• O que a parte " Faça alguma coisa " faz?
• Você reinicia o preenchimento após esta parte?

Estas são apenas algumas perguntas que me vieram à mente quando tentei entender o fragmento de código. Então, na minha opinião, esse é o verdadeiro cheiro do código : seu código não comunica claramente a intenção.

Isso pode significar isso ("tudo ou nada" e atingir o limite indica um erro):

/**
 * 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.
}

Ou pode significar isso ("atualização até o primeiro 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.
}

Ou isto ("atualize todas as listas, mas mantenha a lista original se ela ficar muito 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
}

Ou até o seguinte (usando computeFoos(…)o primeiro exemplo, mas sem exceções):

/**
 * 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);
}

Ou pode significar algo completamente diferente… ;-)