Ist es ein Codegeruch, ein Flag in einer Schleife zu setzen, um es später zu verwenden?

Ich habe ein Stück Code, in dem ich eine Map iteriere, bis eine bestimmte Bedingung erfüllt ist, und diese Bedingung später benutze, um weitere Aufgaben zu erledigen.

Beispiel:

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
}

Ich fühle, dass es ein Code-Geruch ist, eine Flagge zu setzen und diese dann zu verwenden, um mehr Dinge zu tun.

Habe ich recht? Wie könnte ich das entfernen?

Es ist nichts Falsches daran, einen Booleschen Wert für den beabsichtigten Zweck zu verwenden: eine binäre Unterscheidung aufzuzeichnen.

Wenn ich angewiesen würde, diesen Code umzugestalten, würde ich die Schleife wahrscheinlich in eine eigene Methode umwandeln, sodass die Zuweisung + breakin eine return; dann brauchst du nicht einmal eine Variable, kannst du einfach sagen

if(fill_list_from_map()) {
  ...

Es ist nicht unbedingt schlecht, und manchmal ist es die beste Lösung. Das Setzen solcher Flags in verschachtelten Blöcken kann es jedoch schwierig machen, dem Code zu folgen.

Das Problem ist, dass Sie Blöcke zur Begrenzung von Bereichen haben, aber dann Flags, die über Bereiche hinweg kommunizieren und die logische Isolation der Blöcke aufheben. Zum Beispiel limitFlagwird das false sein, wenn das mapis ist null, so dass der "do something" -Code ausgeführt wird, wenn mapis ist null. Dies mag das sein, was Sie beabsichtigen, aber es könnte ein Fehler sein, der leicht zu übersehen ist, da die Bedingungen für dieses Flag an einer anderen Stelle innerhalb eines verschachtelten Bereichs definiert sind. Wenn Sie Informationen und Logik auf ein Mindestmaß beschränken können, sollten Sie dies versuchen.

Ich würde davon abraten, über "Code-Gerüche" nachzudenken. Das ist nur der faulste Weg, um Ihre eigenen Vorurteile zu rationalisieren. Im Laufe der Zeit werden Sie viele Vorurteile entwickeln, und viele von ihnen werden vernünftig sein, aber viele von ihnen werden dumm sein.

Stattdessen sollten Sie praktische (dh nicht dogmatische) Gründe haben, eine Sache der anderen vorzuziehen, und Sie sollten nicht denken, dass Sie für alle ähnlichen Fragen die gleiche Antwort haben sollten.

"Code Gerüche" sind für, wenn Sie nicht sind denken. Wenn Sie wirklich über den Code nachdenken, dann machen Sie es richtig!

In diesem Fall könnte die Entscheidung je nach Umgebungscode in beide Richtungen gehen. Es hängt wirklich davon ab, was Ihrer Meinung nach der klarste Weg ist, über die Funktionsweise des Codes nachzudenken. ("sauberer" Code ist Code, der anderen Entwicklern klar mitteilt, was sie tun, und es ihnen erleichtert, die Richtigkeit zu überprüfen.)

Oft schreiben die Leute Methoden, die in Phasen strukturiert sind, wobei der Code zuerst bestimmt, was er über die Daten wissen muss, und dann darauf reagiert. Wenn sowohl der Teil "Bestimmen" als auch der Teil "Darauf einwirken" etwas kompliziert sind, kann dies sinnvoll sein, und häufig kann das "was es wissen muss" zwischen Phasen in Booleschen Flags übertragen werden. Ich würde es wirklich vorziehen, wenn Sie der Flagge einen besseren Namen gaben. So etwas wie "largeEntryExists" würde den Code viel sauberer machen.

Wenn der Code "// Do Something" dagegen sehr einfach ist, kann es sinnvoller sein, ihn in den ifBlock einzufügen, anstatt ein Flag zu setzen. Dies bringt den Effekt näher an die Ursache und der Leser muss den Rest des Codes nicht scannen, um sicherzustellen, dass das Flag den von Ihnen festgelegten Wert beibehält.

Ja, es ist ein Code-Geruch.

Der Schlüssel für mich ist die Verwendung des break Aussage. Wenn Sie es nicht verwenden, iterieren Sie über mehr Elemente als erforderlich, aber wenn Sie es verwenden, erhalten Sie zwei mögliche Austrittspunkte aus der Schleife.

Kein großes Problem mit Ihrem Beispiel, aber Sie können sich vorstellen, dass es einfacher ist, einen Fehler in den Code zu kriechen, wenn die Bedingungen oder Bedingungen in der Schleife komplexer werden oder die Reihenfolge der Anfangsliste wichtig wird.

Wenn der Code so einfach wie Ihr Beispiel ist, kann er auf eine whileSchleife oder ein äquivalentes Map-Filter-Konstrukt reduziert werden.

Wenn der Code komplex genug ist, um Flags und Breaks zu erfordern, ist er fehleranfällig.

So wie bei allen Code-Gerüchen: Wenn Sie eine Flagge sehen, versuchen Sie, sie durch eine zu ersetzen while. Wenn Sie nicht können, fügen Sie zusätzliche Komponententests hinzu.

Verwenden Sie einfach einen anderen Namen als limitFlag, der angibt, was Sie tatsächlich überprüfen. Und warum protokollierst du etwas, wenn die Karte fehlt oder leer ist? limtFlag wird falsch sein, alles was dir wichtig ist. Die Schleife ist in Ordnung, wenn die Karte leer ist. Sie müssen das also nicht überprüfen.

Einen Booleschen Wert zu setzen, um Informationen zu übermitteln, die Sie bereits hatten, ist meiner Meinung nach eine schlechte Praxis. Wenn es keine einfache Alternative gibt, deutet dies wahrscheinlich auf ein größeres Problem wie eine schlechte Verkapselung hin.

Sie sollten die for-Schleifenlogik in die fillUpList-Methode verschieben, damit sie unterbrochen wird, wenn das Limit erreicht ist. Überprüfen Sie anschließend direkt die Größe der Liste.

Wenn das Ihren Code bricht, warum?

Zunächst zum allgemeinen Fall: Es ist nicht ungewöhnlich, dass ein Flag verwendet wird, um zu überprüfen, ob ein Element einer Sammlung eine bestimmte Bedingung erfüllt. Aber das Muster, das ich am häufigsten gesehen habe, um dies zu lösen, besteht darin, den Scheck in eine zusätzliche Methode zu verschieben und direkt daraus zurückzukehren (wie Kilian Foth in seiner Antwort beschrieben hat ):

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

Seit Java 8 gibt es eine präzisere Möglichkeit, Folgendes zu verwenden Stream.anyMatch(…):

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

In Ihrem Fall würde dies wahrscheinlich so aussehen (vorausgesetzt, list == entry.getValue()in Ihrer Frage):

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

Das Problem in Ihrem konkreten Beispiel ist der zusätzliche Anruf an fillUpList() . Die Antwort hängt stark davon ab, was diese Methode bewirken soll.

Randnotiz: Der Aufruf von fillUpList()macht derzeit wenig Sinn, da er nicht von dem Element abhängt, das Sie gerade iterieren. Ich vermute, dies ist eine Folge davon, dass Sie Ihren eigentlichen Code auf das Frageformat reduziert haben. Aber genau das führt zu einem künstlichen Beispiel, das schwer zu interpretieren und daher schwer zu überlegen ist. Daher ist es so wichtig, ein minimales, vollständiges und überprüfbares Beispiel bereitzustellen .

Ich gehe also davon aus, dass der tatsächliche Code den aktuellen übergibt entry an die Methode weitergibt.

Es gibt jedoch noch weitere Fragen zu stellen:

• Sind die Listen in der Karte leer, bevor dieser Code erreicht wird? Wenn ja, warum gibt es bereits eine Karte und nicht nur die Liste oder den Satz der BigIntegerSchlüssel? Wenn sie nicht leer sind, warum müssen Sie die Listen ausfüllen ? Wenn die Liste bereits Elemente enthält, handelt es sich in diesem Fall nicht um ein Update oder eine andere Berechnung?
• Wodurch wird eine Liste größer als das Limit? Ist dies ein Fehlerzustand oder wird ein häufiges Auftreten erwartet? Wird es durch ungültige Eingabe verursacht?
• Benötigen Sie die Listen, die bis zu dem Punkt berechnet wurden, an dem Sie eine Liste erreichen, die das Limit überschreitet?
• Was macht der Teil " Etwas tun"?
• Starten Sie die Befüllung nach diesem Teil neu?

Dies sind nur einige Fragen, die mir einfielen, als ich versuchte, das Codefragment zu verstehen. Meiner Meinung nach ist das also der wahre Codegeruch : Ihr Code kommuniziert die Absicht nicht klar.

Dies kann entweder bedeuten ("alles oder nichts" und das Erreichen des Grenzwerts weist auf einen Fehler hin):

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

Oder es könnte dies bedeuten ("Update bis zum ersten Problem"):

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

Oder dies ("Aktualisiere alle Listen, aber behalte die Originalliste, wenn sie zu groß wird"):

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

Oder sogar das Folgende (mit computeFoos(…)dem ersten Beispiel, aber ohne Ausnahmen):

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

Oder es könnte etwas ganz anderes bedeuten… ;-)