Invertieren einer IF-Anweisung

39

Ich programmiere jetzt seit ein paar Jahren und habe vor kurzem begonnen, ReSharper mehr zu verwenden. Eine Sache, die ReSharper mir immer vorschlägt, ist, die if-Anweisung zu invertieren, um die Verschachtelung zu reduzieren.

Angenommen, ich habe diesen Code:

foreach (someObject in someObjectList) 
{
    if(someObject != null) 
    {
        someOtherObject = someObject.SomeProperty;
    }
}

Und ReSharper schlägt vor, dass ich das mache:

foreach (someObject in someObjectList)
{
    if(someObject == null) continue;
    someOtherObject = someObject.SomeProperty;
}

Es scheint, dass ReSharper IMMER vorschlägt, dass ich IFs invertiere, egal wie viel verschachtelt wird. Dieses Problem ist, dass ich zumindest in EINIGEN Situationen gerne verschachtele. Für mich scheint es einfacher zu sein, zu lesen und herauszufinden, was an bestimmten Orten vor sich geht. Dies ist nicht immer der Fall, aber ich fühle mich manchmal wohler beim Nisten.

Meine Frage lautet: Gibt es außer persönlichen Vorlieben oder Lesbarkeit einen Grund, die Verschachtelung zu reduzieren? Gibt es einen Leistungsunterschied oder etwas anderes, das mir möglicherweise nicht bewusst ist?

Barry Franklin
quelle
1
Mögliches Duplikat von Einrückungslevel niedrig halten
Gnat
24
Die Schlüsselsache hier ist "Resharper Suggests". Werfen Sie einen Blick auf den Vorschlag. Wenn er die Lesbarkeit und Konsistenz des Codes verbessert, verwenden Sie ihn. Es macht dasselbe mit for / for jeder Schleife.
Jon Raynor
Im Allgemeinen stufe ich diese Resharper-Hinweise herab. Ich befolge sie nicht immer, sodass sie nicht mehr in der Seitenleiste angezeigt werden.
CodesInChaos
1
ReSharper hat das Negativ entfernt, was normalerweise gut ist. Es wurde jedoch keine ternäre Bedingung vorgeschlagen, die hier gut passen könnte, wenn der Block wirklich so einfach ist wie Ihr Beispiel.
Entkorken
2
Schlägt ReSharper nicht auch vor, die Bedingung in die Abfrage zu verschieben? foreach (someObject in someObjectList.Where (object => object! = null)) {...}
Roman Reiner

Antworten:

63

Es hängt davon ab, ob. In Ihrem Beispiel ifist es kein Problem, die nicht invertierte Anweisung zu haben, da der Hauptteil von ifsehr kurz ist. Normalerweise möchten Sie die Anweisungen jedoch umkehren, wenn der Körper wächst.

Wir sind nur Menschen und es ist schwierig, mehrere Dinge gleichzeitig im Kopf zu behalten.

Wenn der Hauptteil einer Anweisung groß ist, verringert das Umkehren der Anweisung nicht nur die Verschachtelung, sondern teilt dem Entwickler auch mit, dass er alles über dieser Anweisung vergessen und sich nur auf den Rest konzentrieren kann. Es ist sehr wahrscheinlich, dass Sie die invertierten Anweisungen verwenden, um falsche Werte so schnell wie möglich zu entfernen. Sobald Sie wissen, dass diese nicht mehr im Spiel sind, bleiben nur noch Werte, die tatsächlich verarbeitet werden können.

Andy
quelle
64
Ich freue mich, wenn die Meinung der Entwickler in diese Richtung geht. Es gibt so viel Verschachtelung im Code, nur weil es eine außergewöhnliche populäre Täuschung gibt break, die continueund mehrere returnnicht strukturiert sind.
JimmyJames
6
Das Problem ist, dass Pause usw. immer noch der Kontrollfluss ist, den Sie im Kopf behalten müssen. "Dies kann nicht x sein, oder es hätte die Schleife oben verlassen." Aber es ist nicht so gut, wenn es nuancierter ist, "nur die Hälfte dieses Codes für diesen Status an Donnerstagen laufen zu lassen"
Ewan
2
@Ewan: Was ist der Unterschied zwischen "Dies kann nicht x sein, oder es hätte die Schleife oben verlassen" und "Dies kann nicht x sein, oder es wäre nicht in diesen Block eingetreten"?
Collin
Nichts ist die Komplexität und Bedeutung des X, die wichtig ist
Ewan
4
@Ewan: Ich denke , break/ continue/ returneinen echten Vorteil gegenüber einem haben elseBlock. Bei einem frühen Ausstieg gibt es zwei Blöcke : den Ausstiegsblock und den Aufenthaltsblock. mit elsegibt es 3 Blöcke : if, else und was auch immer danach kommt (was auch immer der Zweig verwendet wird, der genommen wird). Ich bevorzuge weniger Blöcke.
Matthieu M.
33

Vermeiden Sie nicht Negative. Menschen saugen als sie zu analysieren, auch wenn die CPU sie liebt.

Respektieren Sie jedoch Redewendungen. Wir Menschen sind Gewohnheitstiere, daher bevorzugen wir abgenutzte Pfade gegenüber direkten Pfaden voller Unkräuter.

foo != nullist leider zu einer Redewendung geworden. Die Einzelteile fallen mir kaum noch auf. Ich schaue mir das an und denke nur: "Oh, es ist sicher, dass wir foojetzt abhauen können."

Wenn du das umstürzen willst (oh, eines Tages, bitte), brauchst du einen wirklich starken Fall. Sie brauchen etwas, mit dem meine Augen mühelos davon abrutschen und es sofort verstehen.

Sie gaben uns jedoch Folgendes:

foreach (someObject in someObjectList)
{
    if(someObject == null) continue;
    someOtherObject = someObject.SomeProperty;
}

Welches ist nicht einmal strukturell gleich!

foreach (someObject in someObjectList)
{
    if(someObject == null) continue;
    someOtherObject = someObject.SomeProperty;

    if(someGlobalObject == null) continue;
    someSideEffectObject = someGlobalObject.SomeProperty;
}

Das ist nicht das, was mein fauler Kopf erwartet hatte. Ich dachte, ich könnte weiterhin unabhängigen Code hinzufügen, aber ich kann nicht. Das bringt mich dazu, über Dinge nachzudenken, an die ich jetzt nicht denken möchte. Sie haben meine Redewendung gebrochen, mir aber keine bessere gegeben. Alles nur, weil du mich vor dem einen Negativ beschützen wolltest, das mir ein böses kleines Selbst in die Seele gebrannt hat.

Tut mir leid, vielleicht schlägt ReSharper dies zu 90% der Zeit vor, aber ich denke, Sie haben bei Nullprüfungen einen Eckfall gefunden, in dem es am besten ignoriert wird. Tools können Ihnen einfach nicht beibringen, was lesbarer Code ist. Frage einen Menschen. Führen Sie eine Begutachtung durch. Aber folge dem Werkzeug nicht blindlings.

kandierte_orange
quelle
1
Wenn Sie mehr Code in der Schleife haben, hängt die Funktionsweise davon ab, wie alles zusammenkommt. Es ist kein unabhängiger Code. Der überarbeitete Code in der Frage stimmte für das Beispiel, ist aber möglicherweise nicht korrekt, wenn Sie nicht isoliert sind. War dies der Punkt, für den Sie sich entschieden haben? (+1 für die nicht vermeiden, aber Negative)
Baldrickk
Mit @Baldrickk kann if (foo != null){ foo.something() }ich weiterhin Code hinzufügen, ohne mich darum zu kümmern, ob foonull war. Das tut es nicht. Auch wenn ich das jetzt nicht tun muss, fühle ich mich nicht motiviert, die Zukunft zu vermasseln, indem ich sage: "Nun, sie können es einfach umgestalten, wenn sie das brauchen." Feh. Software ist nur dann weich, wenn sie leicht zu ändern ist.
candied_orange
4
"Code immer wieder hinzufügen, ohne sich darum zu kümmern" Entweder sollte der Code in Beziehung gesetzt werden (andernfalls sollte er wahrscheinlich getrennt sein) oder es sollte sorgfältig darauf geachtet werden, die Funktionalität der zu ändernden Funktion / des zu ändernden Blocks zu verstehen, da das Hinzufügen von Code möglicherweise das Verhalten ändert darüber hinaus beabsichtigt. Für einfache Fälle wie diesen denke ich nicht, dass es so oder so wichtig ist. Für komplexere Fälle würde ich erwarten, dass das Verständnis des Codes Vorrang hat, sodass ich wählen würde, was meiner Meinung nach am klarsten ist und welcher der beiden Stile sein könnte.
Baldrickk
verwandt und verflochten sind nicht dasselbe.
candied_orange
1
Vermeiden Sie nicht Negative: D
VitalyB
20

Es ist das alte Argument, ob eine Pause schlimmer ist als ein Nest.

Die Leute scheinen eine frühzeitige Rückgabe für Parameterprüfungen zu akzeptieren, aber die meisten Methoden sind verpönt.

Persönlich vermeide ich, fortzufahren, zu brechen, usw. zu gehen, auch wenn es mehr Verschachtelung bedeutet. In den meisten Fällen können Sie jedoch beides vermeiden.

foreach (someObject in someObjectList.where(i=>i != null))
{
    someOtherObject = someObject.SomeProperty;
}

Offensichtlich erschwert das Verschachteln die Verfolgung, wenn Sie viele Ebenen haben

foreach (someObject in someObjectList) 
{
    if(someObject != null) 
    {
        someOtherObject = someObject.SomeProperty;
        if(someObject.x > 5) 
        {
            x ++;
            if(someObject.y > 5) 
            {
                x ++;
            }
        }
    }
}

Das Schlimmste ist, wenn Sie beide haben

foreach (someObject in someObjectList) 
{
    if(someObject != null) 
    {
        someOtherObject = someObject.SomeProperty;
        if(someObject.x > 5) 
        {
            x ++;
            if(someObject.y > 5) 
            {
                x ++;
                return;
            }
            continue;
        }
        someObject.z --;
        if(myFunctionWithSideEffects(someObject) > 6) break;
    }
}
Ewan
quelle
11
Liebe diese Zeile: foreach (subObject in someObjectList.where(i=>i != null))Weiß nicht, warum ich noch nie so etwas gedacht habe.
Barry Franklin
Bei @BarryFranklin ReSharper sollte auf der Foreach-Seite eine grün gepunktete Unterstreichung mit "In LINQ-Ausdruck konvertieren" als Vorschlag angezeigt werden, der dies für Sie erledigt. Abhängig von der Operation werden möglicherweise auch andere linq-Methoden wie Sum oder Max ausgeführt.
Kevin Fee
@BarryFranklin Es ist wahrscheinlich ein Vorschlag, den Sie auf ein niedriges Niveau einstellen und nur nach Belieben verwenden möchten. Während es in einfachen Fällen wie diesem Beispiel gut funktioniert, finde ich in komplexerem Code die Art und Weise, in der komplexere Bedingungen in Teile zerlegt werden, die nicht mehr zu verstehen sind. Ich werde den Vorschlag im Allgemeinen zumindest versuchen, da die Ergebnisse oft vernünftig sind, wenn eine gesamte Schleife in Linq konvertiert werden kann. aber halb und halb Ergebnisse neigen dazu, hässlich schnell IMO zu bekommen.
Dan Neely
Ironischerweise hat die Bearbeitung durch Nachschärfen genau die gleiche Verschachtelungsebene, da es auch ein Wenn gefolgt von einem Einzeiler gibt.
Peter
heh, keine geschweiften Klammern! was anscheinend erlaubt ist: /
Ewan
6

Das Problem dabei continueist, dass die Logik kompliziert wird und wahrscheinlich Fehler enthält, wenn Sie dem Code weitere Zeilen hinzufügen. z.B

foreach (someObject in someObjectList)
{
    if(someObject == null) continue;
    someOtherObject = someObject.SomeProperty;

    ...  imagine that there is some code here ...

    // added later by a Junior Developer
    doSomeOtherFunction(someObject);
}

Möchten Sie doSomeOtherFunction()für alle someObject aufrufen oder nur, wenn nicht null? Mit dem Originalcode haben Sie die Möglichkeit und es wird klarer. Mit dem continuekönnte man vergessen "oh, yeah, da hinten haben wir Nullen übersprungen" und Sie haben nicht einmal die Möglichkeit, es für alle someObjects aufzurufen, es sei denn, Sie setzen diesen Aufruf am Anfang der Methode, was möglicherweise nicht möglich oder korrekt ist aus anderen gründen.

Ja, viel Schachteln ist hässlich und möglicherweise verwirrend, aber in diesem Fall würde ich den traditionellen Stil verwenden.

Bonusfrage : Warum enthält diese Liste zunächst Nullen? Es ist möglicherweise besser, überhaupt keine Null hinzuzufügen. In diesem Fall ist die Verarbeitungslogik viel einfacher.

user949300
quelle
12
Es sollte nicht genug Code in der Schleife geben, dass Sie die Nullprüfung oben nicht sehen können, ohne zu scrollen.
Bryan Boettcher
@ Bryan Boettcher stimmte zu, aber nicht jeder Code ist so gut geschrieben. Und selbst mehrere Zeilen komplexen Codes können dazu führen, dass man das Fortfahren vergisst (oder missachtet). In der Praxis bin ich in Ordnung, returnweil sie eine "saubere Pause" bilden, aber IMO breakund continuezu Verwirrung führen und in den meisten Fällen am besten vermieden werden.
user949300
4
@ user949300 Nach zehnjähriger Entwicklung habe ich noch nie eine Unterbrechung festgestellt oder weiterhin Verwirrung gestiftet. Ich hatte sie in Verwirrung verwickelt, aber sie waren nie die Ursache. In der Regel waren schlechte Entscheidungen des Entwicklers die Ursache und hätten für Verwirrung gesorgt, egal welche Waffe sie verwendeten.
Corsika
1
@corsiKa Der Apple SSL-Bug ist eigentlich ein Goto-Bug, könnte aber leicht ein Break oder Continue- Bug gewesen sein. Allerdings ist der Code schrecklich ...
user949300
3
@BryanBoettcher Das Problem scheint mir, dass die gleichen Entwickler, die weiterhin denken oder mehrere Rückgaben in Ordnung sind, auch denken, sie in großen Methodenkörpern zu vergraben, ist auch in Ordnung. Jede Erfahrung, die ich mit Continue / Multiple Returns gemacht habe, war in einem großen Chaos von Code.
Andy
3

Die Idee ist die frühe Rückkehr. Früh returnin einer Schleife wird früh continue.

Viele gute Antworten auf diese Frage: Soll ich frühzeitig von einer Funktion zurückkehren oder eine if-Anweisung verwenden?

Beachten Sie, dass 430+ Stimmen für eine vorzeitige Rückkehr sind, ~ 50 Stimmen für eine vorzeitige Rückkehr und 8 gegen eine vorzeitige Rückkehr. Es besteht also ein außergewöhnlich starker Konsens (entweder das, oder ich zähle schlecht, was plausibel ist).

Persönlich neige ich dazu, den Schleifenkörper in eine Funktion zu extrahieren, in der ich Early Return anstelle von Early Continue verwende.

Peter
quelle
2

Diese Technik ist nützlich, um Vorbedingungen zu zertifizieren, wenn der Hauptteil Ihrer Methode auftritt, wenn diese Bedingungen erfüllt sind.

ifsBei meiner Arbeit kehren wir häufig um und setzen ein Phantom ein. Früher war es ziemlich häufig, dass ich beim Überprüfen einer PR darauf hinweisen konnte, wie mein Kollege drei Verschachtelungsebenen entfernen konnte! Es kommt seltener vor, da wir unser Wenn ausrollen.

Hier ist ein Spielzeugbeispiel für etwas, das ausgerollt werden kann

function foobar(x, y, z) {
    if (x > 0) {
        if (z != null && isGamma(y)) {
            // ~10 lines of code
            // return something
        }
        else {
           return y
        }
    }
    else {
      return y + z
    }
}

Code wie dieser, in dem es EINEN interessanten Fall gibt (in dem der Rest einfach Rückgaben oder kleine Anweisungsblöcke sind), ist weit verbreitet. Es ist trivial, das oben Gesagte als umzuschreiben

function foobar(x, y, z) {
    if (x <=0) {
        return y + z
    }
    if (z == null || !isGamma(y) {
       return y
    }
    // ~ 10 lines of code
    // return something
}

Hier wird unser signifikanter Code, die nicht eingeschlossenen 10 Zeilen, in der Funktion nicht dreifach eingerückt. Wenn Sie den ersten Stil haben, sind Sie entweder versucht, den langen Block in eine Methode umzugestalten, oder Sie tolerieren das Einrücken. Hier setzt die Einsparung ein. An einer Stelle ist dies eine triviale Einsparung, aber bei vielen Methoden in vielen Klassen sparen Sie sich die Notwendigkeit, eine zusätzliche Funktion zu generieren, um den Code sauberer zu machen, und Sie haben nicht so viel abscheuliche Mehrebenen Einrückung .


Als Reaktion auf OPs Codebeispiel stimme ich zu, dass dies ein übermäßiger Zusammenbruch ist. Besonders, da in 1 TB, dem von mir verwendeten Stil, die Inversion dazu führt, dass der Code größer wird.

Lan
quelle
0

Resharper-Vorschläge sind oft nützlich, sollten aber immer kritisch bewertet werden. Es sucht nach vordefinierten Codemustern und schlägt Transformationen vor, kann jedoch nicht alle Faktoren berücksichtigen, die den Code für den Menschen mehr oder weniger lesbar machen.

Wenn alle anderen Dinge gleich sind, ist eine geringere Verschachtelung vorzuziehen, weshalb ReSharper eine Transformation vorschlägt, die die Verschachtelung verringert. Aber auch alle anderen Dinge sind nicht gleich: In diesem Fall wird die Transformation die Einführung eines erfordert continue, was bedeutet , der resultierende Code ist tatsächlich schwieriger zu folgen.

In einigen Fällen ein Niveaus der Verschachtelung für einen Austausch continue könnte in der Tat eine Verbesserung sein, aber das hängt von der Semantik des Codes in Frage, die über das Verständnis von ReSharpers mechanischen Regeln.

In Ihrem Fall würde der ReSharper-Vorschlag den Code verschlimmern. Also ignoriere es einfach. Oder besser: Verwenden Sie nicht die vorgeschlagene Transformation, sondern überlegen Sie, wie der Code verbessert werden könnte. Beachten Sie, dass Sie möglicherweise dieselbe Variable mehrmals zuweisen, aber nur die letzte Zuweisung wirkt sich aus, da die vorherige nur überschrieben wird. Das sieht nach einem Bug aus. Der Vorschlag von ReSharpers behebt den Fehler nicht, macht jedoch deutlich, dass eine zweifelhafte Logik im Gange ist.

JacquesB
quelle
0

Ich denke, der größere Punkt hier ist, dass der Zweck der Schleife den besten Weg vorgibt, den Fluss innerhalb der Schleife zu organisieren. Wenn Sie einige Elemente herausfiltern, bevor Sie die verbleibenden Elemente durchlaufen, continueist es sinnvoll, sie vorzeitig zu entfernen. Wenn Sie jedoch verschiedene Arten der Verarbeitung in einer Schleife ausführen, ist es möglicherweise sinnvoller, dies ifwirklich deutlich zu machen , z. B .:

for(obj in objectList) {
    if(obj == null) continue;
    if(obj.getColour().equals(Color.BLUE)) {
        // Handle blue objects
    } else {
        // Handle non-blue objects
    }
}

Beachten Sie, dass Sie in Java Streams oder anderen funktionalen Idiomen die Filterung von der Schleife trennen können, indem Sie Folgendes verwenden:

items.stream()
    .filter(i -> (i != null))
    .filter(i -> i.getColour().equals(Color.BLUE))
    .forEach(i -> {
        // Process blue objects
    });

Übrigens ist dies eines der Dinge, die ich an Perls invertiertem if ( unless) liebe , das auf einzelne Anweisungen angewendet wird und das die folgende Art von Code ermöglicht, die ich sehr einfach zu lesen finde:

foreach my $item (@items) {
    next unless defined $item;
    carp "Only blue items are supported! Failed on item $item" 
       unless ($item->get_colour() eq 'blue');
    ...
}
Gaurav
quelle