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

30

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?

Siddharth Trikha
quelle
10
Warum glaubst du, dass es ein Codegeruch ist? Welche spezifischen Probleme können Sie dabei vorhersehen, die unter einer anderen Struktur nicht auftreten würden?
Ben Cottrell
13
@ gnasher729 Nur aus Neugierde, welchen Begriff würdest du stattdessen verwenden?
Ben Cottrell
11
-1, Ihr Beispiel macht keinen Sinn. entrywird in der Funktionsschleife nirgends verwendet, und wir können nur raten, was listist. Soll fillUpListfüllen list? Warum bekommt es es nicht als Parameter?
Doc Brown
13
Ich würde Ihre Verwendung von Leerzeichen und Leerzeilen überdenken.
Daniel Jour
11
Code riecht nicht. "Codegeruch" ist ein Begriff, der von Softwareentwicklern erfunden wurde, die ihre Nase halten möchten, wenn sie Code sehen, der nicht ihren elitären Standards entspricht.
Robert Harvey

Antworten:

70

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()) {
  ...
Kilian Foth
quelle
6
Tatsächlich ist der Geruch in seinem Code die lange Funktion, die in kleinere Funktionen aufgeteilt werden muss. Ihr Vorschlag ist der richtige Weg.
Bernhard Hiller
2
Ein besserer Ausdruck, der die nützliche Funktion des ersten Teils dieses Codes beschreibt, ist das Ermitteln, ob das Limit überschritten wird, nachdem etwas aus diesen zugeordneten Elementen gesammelt wurde. Wir können auch davon ausgehen, dass fillUpList()es sich um einen Code handelt (den OP nicht freigibt), der tatsächlich den Wert entryaus der Iteration verwendet. Ohne diese Annahme würde es so aussehen, als ob der Schleifenkörper nichts von der Schleifeniteration verwendet hätte.
Rwong
4
@Kilian: Ich habe nur eine Sorge. Diese Methode füllt eine Liste auf und gibt einen Booleschen Wert zurück, der diese Listengröße repräsentiert, die ein Limit überschreitet oder nicht. Der Name 'fill_list_from_map' verdeutlicht also nicht, was der zurückgegebene Boolesche Wert darstellt (er konnte nicht gefüllt werden, a Limit überschreitet, etc). Als zurückgegebener Boolescher Wert gilt ein Sonderfall, der aus dem Funktionsnamen nicht ersichtlich ist. Irgendwelche Kommentare ? PS: Wir können auch die Trennung von Befehlsabfragen berücksichtigen.
Siddharth Trikha
2
@SiddharthTrikha Sie haben Recht, und ich hatte genau die gleiche Sorge, als ich diese Zeile vorschlug. Es ist jedoch unklar, welche Liste der Code füllen soll. Wenn es sich immer um dieselbe Liste handelt, benötigen Sie die Flagge nicht, sondern können ihre Länge nachträglich überprüfen. Wenn Sie wissen müssen, ob eine einzelne Auffüllung das Limit überschritten hat, müssen Sie diese Informationen irgendwie nach außen transportieren, und IMO ist das Prinzip der Trennung von Befehlen und Abfragen kein ausreichender Grund, um den offensichtlichen Weg abzulehnen: über die Rückgabe Wert.
Kilian Foth
6
Onkel Bob sagt auf Seite 45 von Clean Code : "Funktionen sollten entweder etwas tun oder etwas beantworten, aber nicht beides. Entweder sollte Ihre Funktion den Status eines Objekts ändern, oder sie sollte einige Informationen zu diesem Objekt zurückgeben. Beides führt häufig zu Verwechslung."
CJ Dennis
25

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.

JacquesB
quelle
2
Dies war der Grund, warum ich den Eindruck hatte, dass es sich um einen Codegeruch handelt, da die Blöcke nicht vollständig isoliert sind und sich später nur schwer nachverfolgen lassen. Also denke ich, dass der Code in @Kilians Antwort der beste ist, den wir bekommen können?
Siddharth Trikha
1
@SiddharthTrikha: Es ist schwer zu sagen, da ich nicht weiß, was der Code eigentlich tun soll. Wenn Sie nur überprüfen möchten, ob die Map mindestens ein Element enthält, dessen Liste größer als der Grenzwert ist, können Sie dies meines Erachtens mit einem einzelnen anyMatch-Ausdruck tun.
JacquesB
2
@SiddharthTrikha: Das Problem mit dem Gültigkeitsbereich kann leicht gelöst werden, indem der ursprüngliche Test in eine Schutzklausel wie if(map==null || map.isEmpty()) { logger.info(); return;}diese geändert wird. Dies funktioniert jedoch nur, wenn der angezeigte Code der gesamte Funktionskörper ist und der // Do somethingTeil für die Zuordnung nicht erforderlich ist ist null oder leer.
Doc Brown
14

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.

Matt Timmermans
quelle
5

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.

Ewan
quelle
+1 von mir. Es ist mit Sicherheit ein Codegeruch, und Sie artikulieren, warum und wie Sie damit umgehen sollen.
David Arno
@Ewan: SO as with all code smells: If you see a flag, try to replace it with a whileKannst du das an einem Beispiel erläutern?
Siddharth Trikha
2
Mehrere Austrittspunkte aus der Schleife hat , kann es schwieriger zu Grunde über, aber in diesem Fall wäre es so Refactoring auf die Schleifenbedingung auf der Flagge abhängig zu machen - es bedeuten würde Ersatz for (Map.Entry<BigInteger, List<String>> entry : map.entrySet())mit for (Iterator<Map.Entry<BigInteger, List<String>>> iterator = map.entrySet().iterator(); iterator.hasNext() && !limitFlag; Map.Entry<BigInteger, List<String>> entry = iterator.next()). Das ist ein Muster, das selten genug ist, dass ich mehr Probleme haben würde, es zu verstehen, als eine relativ einfache Pause.
James_pic
@James_pic Mein Java ist ein bisschen verrostet, aber wenn wir Karten verwenden, würde ich einen Sammler verwenden, um die Anzahl der Elemente zusammenzufassen und diese nach dem Limit herauszufiltern. Wie ich jedoch sage, ist das Beispiel "nicht so schlimm", dass ein Codegeruch eine allgemeine Regel ist, die Sie vor einem möglichen Problem warnt. Kein heiliges Gesetz, dem Sie immer gehorchen müssen
Ewan
1
Meinst du nicht "Queue" statt "Queue"?
Psmears
0

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.

gnasher729
quelle
0

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?

user294250
quelle
0

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… ;-)

siegi
quelle