È un odore del codice impostare un flag in un ciclo per usarlo in seguito?

30

Ho un pezzo di codice in cui eseguo l'iterazione di una mappa fino a quando una determinata condizione è vera e poi in seguito uso quella condizione per fare altre cose.

Esempio:

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
}

Mi sento di impostare una bandiera e quindi usarla per fare più cose è un odore di codice.

Ho ragione? Come posso rimuovere questo?

    
posta Siddharth Trikha 22.01.2018 - 08:21
fonte

7 risposte

70

Non c'è niente di sbagliato nell'usare un valore booleano per lo scopo previsto: registrare una distinzione binaria.

Se mi venisse detto di refactoring questo codice, probabilmente metterei il ciclo in un metodo a sé stante in modo che l'assegnazione + break si trasformi in return ; quindi non hai nemmeno bisogno di una variabile, puoi semplicemente dire

if(fill_list_from_map()) {
  ...
    
risposta data 22.01.2018 - 08:48
fonte
25

Non è necessariamente negativo, ea volte è la soluzione migliore. Ma impostare flag come questo in blocchi nidificati può rendere il codice difficile da seguire.

Il problema è che hai dei blocchi per delimitare gli ambiti, ma poi hai flag che comunicano attraverso gli scope, rompendo l'isolamento logico dei blocchi. Ad esempio, limitFlag sarà false se map è null , quindi il codice "fa qualcosa" verrà eseguito se map è null . Questo potrebbe essere ciò che intendete, ma potrebbe essere un bug che è facile perdere, perché le condizioni per questo flag sono definite da qualche altra parte, all'interno di un ambito nidificato. Se riesci a mantenere le informazioni e la logica nel più stretto ambito possibile, dovresti tentare di farlo.

    
risposta data 22.01.2018 - 08:56
fonte
14

Consiglierei di non ragionare sugli "odori di codice". Questo è solo il modo più pigro possibile per razionalizzare i tuoi pregiudizi. Col tempo svilupperai molti pregiudizi, e molti di essi saranno ragionevoli, ma molti di loro saranno stupidi.

Invece, dovresti avere ragioni pratiche (cioè non dogmatiche) per preferire una cosa piuttosto che un'altra, ed evitare di pensare che dovresti avere la stessa risposta per tutte le domande simili.

"Gli odori di codice" si riferiscono a quando non stai pensando. Se penserai davvero al codice, fallo bene!

In questo caso, la decisione potrebbe davvero andare in entrambi i modi a seconda del codice circostante. Dipende davvero da quello che pensi sia il modo più chiaro per pensare a ciò che sta facendo il codice. (Il codice "pulito" è un codice che comunica chiaramente ciò che sta facendo agli altri sviluppatori e rende facile per loro verificare che sia corretto)

Un sacco di volte, le persone scriveranno metodi strutturati in fasi, in cui il codice determinerà prima ciò che ha bisogno di sapere sui dati e poi agirà su di esso. Se la parte "determinante" e la parte "recita" sono entrambe un po 'complicate, allora può essere logico farlo, e spesso "ciò che deve sapere" può essere trasportato tra le fasi in flag booleani. Preferirei davvero che tu abbia dato un nome migliore alla bandiera, comunque. Qualcosa come "largeEntryExists" renderebbe il codice molto più pulito.

Se, d'altra parte, il codice "// Fai qualcosa" è molto semplice, allora può avere più senso inserirlo nel blocco if invece di impostare un flag. Questo avvicina l'effetto alla causa e il lettore non deve eseguire la scansione del resto del codice per assicurarsi che il flag mantenga il valore che si desidera impostare.

    
risposta data 22.01.2018 - 14:34
fonte
5

Sì, è un odore di codice (cita i downvotes di tutti quelli che lo fanno).

La cosa fondamentale per me è l'uso dell'istruzione break . Se non lo utilizzi, eseguiresti l'iterazione di più elementi del necessario, ma utilizzandoli fornisci due possibili punti di uscita dal ciclo.

Non è un grosso problema con il tuo esempio, ma puoi immaginare che quando le condizioni condizionali o condizionali all'interno del ciclo diventano più complesse o l'ordinamento della lista iniziale diventa importante, allora è più facile che un bug si insinui nel codice.

Quando il codice è semplice come il tuo esempio, può essere ridotto a un ciclo while o a una mappa equivalente, costrutto del filtro.

Quando il codice è abbastanza complesso da richiedere flag e interruzioni, sarà soggetto a bug.

Così come per tutti gli odori di codice: se vedi una bandiera, prova a sostituirla con una while . Se non puoi, aggiungi ulteriori test unitari.

    
risposta data 22.01.2018 - 10:37
fonte
0

Usa solo un nome diverso da limitFlag che indica cosa stai effettivamente controllando. E perché registri qualcosa quando la mappa è assente o vuota? limtFlag sarà falso, tutto ciò che ti interessa. Il ciclo va bene se la mappa è vuota, quindi non c'è bisogno di controllarla.

    
risposta data 22.01.2018 - 08:57
fonte
0

Impostare un valore booleano per trasmettere le informazioni che hai già è una cattiva pratica secondo me. Se non esiste un'alternativa semplice, è probabilmente indicativo di un problema più grande come l'incapsulamento scadente.

Dovresti spostare la logica del ciclo for nel metodo fillUpList per interromperla se viene raggiunto il limite. Quindi controlla la dimensione della lista subito dopo.

Se questo rompe il tuo codice, perché?

    
risposta data 24.01.2018 - 16:33
fonte
0

Prima di tutto il caso generale: usare un flag per verificare se alcuni elementi di una collezione soddisfano una determinata condizione non è raro. Ma lo schema che ho visto più spesso per risolverlo è spostare il controllo in un metodo extra e ritornare direttamente da esso (come Kilian Foth descritto in la sua risposta ):

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

Da Java 8 c'è un modo più conciso usando Stream.anyMatch(…) :

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

Nel tuo caso questo probabilmente sarebbe simile a questo (assumendo list == entry.getValue() nella tua domanda):

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

Il problema nel tuo esempio specifico è la chiamata aggiuntiva a fillUpList() . La risposta dipende molto da ciò che questo metodo dovrebbe fare.

Nota a margine: così com'è, la chiamata a fillUpList() non ha molto senso, perché non dipende dall'elemento che stai attualmente iterando. Immagino che questa sia una conseguenza dell'eliminazione del codice effettivo per adattarlo al formato della domanda. Ma proprio questo porta ad un esempio artificiale che è difficile da interpretare e quindi difficile da ragionare. Pertanto è molto importante fornire un minimo, Completo e Verificabile esempio .

Quindi presumo che il codice effettivo passi l'attuale entry al metodo.

Ma ci sono altre domande da porre:

  • Gli elenchi nella mappa sono vuoti prima di raggiungere questo codice? In tal caso, perché esiste già una mappa e non solo la lista o il set delle chiavi BigInteger ? Se non sono vuoti, perché è necessario compilare gli elenchi? Quando ci sono già elementi nella lista, non è un aggiornamento o qualche altro calcolo in questo caso?
  • Che cosa fa sì che una lista diventi più grande del limite? Si tratta di una condizione di errore o si prevede che si verifichi spesso? È causato da un input non valido?
  • Hai bisogno delle liste calcolate fino al punto in cui raggiungi una lista più grande del limite?
  • Che cosa fa la parte " Fai qualcosa "?
  • Riavvia il materiale di riempimento dopo questa parte?

Queste sono solo alcune domande che mi sono venute in mente quando ho cercato di capire il frammento di codice. Quindi, a mio parere, questo è il vero odore del codice : il tuo codice non comunica chiaramente l'intento.

Potrebbe significare questo ("tutto o niente" e il raggiungimento del limite indica un errore):

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

Oppure potrebbe significare questo ("aggiorna fino al primo 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 questo ("aggiorna tutti gli elenchi ma mantieni l'elenco originale se diventa troppo 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 anche i seguenti (utilizzando computeFoos(…) dal primo esempio ma senza eccezioni):

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

Oppure potrebbe significare qualcosa di completamente diverso ...; -)

    
risposta data 27.01.2018 - 06:00
fonte