Eleganter Umgang mit if (falls sonst) else

161

Dies ist ein kleiner Witz, aber jedes Mal, wenn ich so etwas codieren muss, stört mich die Wiederholung, aber ich bin nicht sicher, ob eine der Lösungen nicht schlechter ist.

if(FileExists(file))
{
    contents = OpenFile(file); // <-- prevents inclusion in if
    if(SomeTest(contents))
    {
        DoSomething(contents);
    }
    else
    {
        DefaultAction();
    }
}
else
{
    DefaultAction();
}
  • Gibt es einen Namen für diese Art von Logik?
  • Bin ich ein bisschen zu OCD?

Ich bin offen für böse Codevorschläge, wenn auch nur aus Neugier ...

Benjol
quelle
8
@Emmad Kareem: Zwei DefaultActionAnrufe verletzen das DRY-Prinzip
Abyx
Vielen Dank für Ihre Antwort, aber ich denke, dass es in Ordnung ist, außer dass Sie try / catch nicht verwenden, da es möglicherweise Fehler gibt, die keine Ergebnisse liefern und Abbrüche verursachen würden (abhängig von Ihrer Programmiersprache).
NoChance
20
Ich denke, das Hauptproblem hierbei ist, dass Sie auf inkonsistenten Abstraktionsebenen arbeiten . Die höhere Abstraktionsebene ist: make sure I have valid data for DoSomething(), and then DoSomething() with it. Otherwise, take DefaultAction(). Die wichtigsten Details, um sicherzustellen, dass Sie über die Daten für DoSomething () verfügen, befinden sich auf einer niedrigeren Abstraktionsebene und sollten daher eine andere Funktion haben. Diese Funktion hat einen Namen auf der höheren Abstraktionsebene, und ihre Implementierung erfolgt auf niedriger Ebene. Die folgenden guten Antworten sprechen dieses Problem an.
Gilad Naor
6
Bitte geben Sie eine Sprache an. Mögliche Lösungen, Standard-Redewendungen und langjährige kulturelle Normen sind für verschiedene Sprachen unterschiedlich und führen zu unterschiedlichen Antworten auf Ihre Frage.
Caleb
1
Sie können auf dieses Buch "Refactoring: Verbessern des Entwurfs von vorhandenem Code" verweisen. Es gibt mehrere Abschnitte über die If-else-Struktur, eine wirklich nützliche Vorgehensweise.
Vacker

Antworten:

96

Extrahieren Sie es, um die Funktion (Methode) und die returnAnweisung use zu trennen :

if(FileExists(file))
{
    contents = OpenFile(file); // <-- prevents inclusion in if
    if(SomeTest(contents))
    {
        DoSomething(contents);
        return;
    }
}

DefaultAction();

Oder, vielleicht besser, trennen Sie das Abrufen von Inhalten und deren Verarbeitung:

contents_t get_contents(name_t file)
{
    if(!FileExists(file))
        return null;

    contents = OpenFile(file);
    if(!SomeTest(contents)) // like IsContentsValid
        return null;

    return contents;
}

...

contents = get_contents(file)
contents ? DoSomething(contents) : DefaultAction();

Update:

Warum keine Ausnahmen, warum OpenFilekeine
E / A- Ausnahme: Ich denke, es handelt sich eher um eine allgemeine Frage als um eine Frage zu Datei-E / A. Namen wie FileExists, OpenFilekönnen verwirrend sein , aber wenn sie mit zu ersetzen Foo, Barusw. - es wäre klarer , dass DefaultActionwie so oft genannt werden kann DoSomething, so dass es nicht-Ausnahmefall sein kann. Péter Török schrieb am Ende seiner Antwort darüber

Warum es in der 2. Variante einen ternären bedingten Operator
gibt : Wenn es ein [C ++] - Tag geben würde, würde ich eine ifAnweisung mit der Deklaration von contentsin ihrem Bedingungsteil schreiben:

if(contents_t contents = get_contents(file))
    DoSomething(contents);
else
    DefaultAction();

Aber für andere (C-ähnliche) Sprachen if(contents) ...; else ...;ist genau das gleiche wie Ausdrucksanweisung mit ternärem Bedingungsoperator, aber länger. Da der Hauptteil des Codes die get_contentsFunktion war, habe ich nur die kürzere Version verwendet (und auch contentsTyp weggelassen ). Jedenfalls steht es außer Frage.

Abyx
quelle
93
+1 für mehrere zurückkehrt - wenn Methoden sind ausreichend klein gemacht , arbeitete dieser Ansatz am besten für mich
gnat
Kein großer Fan der Mehrfachrückgaben, obwohl ich es gelegentlich benutze. Es ist ziemlich vernünftig für etwas Einfaches, aber skaliert nicht gut. Unser Standard ist es, es für alle außer verrückten einfachen Methoden zu vermeiden, da Methoden dazu neigen, mehr an Größe zuzunehmen als zu schrumpfen.
Brian Knoblauch
3
Mehrere Rückgabepfade können negative Auswirkungen auf die Leistung in C ++ - Programmen haben, was die Bemühungen des Optimierers, RVO zu verwenden, zunichte macht (auch NRVO, sofern nicht jeder Pfad dasselbe Objekt zurückgibt).
Funktastischer
Ich würde empfehlen, die Logik für die 2. Lösung umzukehren: {if (file exists) {set contents; if (sometest) {Inhalt zurückgeben; }} return null; } Es vereinfacht den Ablauf und reduziert die Anzahl der Zeilen.
Wedge
1
Hallo Abyx, mir ist aufgefallen, dass du einige Rückmeldungen aus den Kommentaren hier aufgenommen hast: Danke, dass du das getan hast. Ich habe alles aufgeräumt, was in Ihrer Antwort und in anderen Antworten angesprochen wurde.
56

Wenn die von Ihnen verwendete Programmiersprache (0) binäre Vergleiche kurzschließt (dh wenn nicht aufgerufen wird, SomeTestwenn FileExistsfalsch zurückgegeben wird) und (1) Zuweisung einen Wert zurückgibt (das Ergebnis von OpenFilewird zugewiesen, contentsund dieser Wert wird dann als Argument übergeben Bis SomeTest) können Sie so etwas wie das Folgende verwenden, aber es wird Ihnen dennoch empfohlen, den Code zu kommentieren und darauf hinzuweisen, dass die Single =beabsichtigt ist.

if( FileExists(file) && SomeTest(contents = OpenFile(file)) )
{
    DoSomething(contents);
}
else
{
    DefaultAction();
}

Je nachdem, wie kompliziert das if ist, ist es möglicherweise besser, eine Flag-Variable zu haben (die das Testen von Erfolgs- / Fehlerbedingungen mit dem Code trennt, der DefaultActionin diesem Fall den Fehler behandelt ).

frozenkoi
quelle
So würde ich es machen.
Anthony
13
ifMeiner Meinung nach ist es ziemlich eklig, so viel Code in eine Aussage zu setzen .
moteutsch
15
Im Gegenteil, ich mag diese Art der Aussage "Wenn etwas existiert und diese Bedingung erfüllt". +1
Gorpik
Ich auch! Ich persönlich mag es nicht, wie Menschen mehrere Retouren verwenden, wenn bestimmte Voraussetzungen nicht erfüllt sind. Warum Sie nicht diese ifs invertieren und Ihren Code ausführen , wenn sie werden erfüllt?
klaar
"Wenn etwas existiert und diese Bedingung erfüllt" ist in Ordnung. "Wenn etwas existiert und hier etwas tangential Verbundenes tut und diese Bedingung erfüllt", ist OTOH verwirrend. Mit anderen Worten, ich mag keine Nebenwirkungen in einem Zustand.
Piskvor
26

Ernsthafter als die Wiederholung des Aufrufs von DefaultAction ist der Stil selbst, da der Code nicht orthogonal geschrieben ist ( gute Gründe für das orthogonale Schreiben finden Sie in dieser Antwort ).

Um zu zeigen, warum nicht-orthogonaler Code schlecht ist, betrachten Sie das ursprüngliche Beispiel, wenn eine neue Anforderung eingeführt wird, dass die Datei nicht geöffnet werden darf, wenn sie auf einer Netzwerkfestplatte gespeichert ist. Nun, dann können wir den Code einfach wie folgt aktualisieren:

if(FileExists(file))
{
    if(! OnNetworkDisk(file))
    {
        contents = OpenFile(file); // <-- prevents inclusion in if
        if(SomeTest(contents))
        {
            DoSomething(contents);
        }
        else
        {
            DefaultAction();
        }
    }
    else
    {
        DefaultAction();
    }
}
else
{
    DefaultAction();
}

Dann kommt aber auch die Forderung, dass wir auch keine großen Dateien über 2 GB öffnen dürfen. Nun, wir aktualisieren gerade noch einmal:

if(FileExists(file))
{
    if(LessThan2Gb(file))
    {
        if(! OnNetworkDisk(file))
        {
            contents = OpenFile(file); // <-- prevents inclusion in if
            if(SomeTest(contents))
            {
                DoSomething(contents);
            }
            else
            {
                DefaultAction();
            }
        }
        else
        {
            DefaultAction();
        }
    else
    {
        DefaultAction();
    }
}
else
{
    DefaultAction();
}

Es sollte klar sein, dass ein derartiger Codestil einen enormen Wartungsaufwand darstellt.

Zu den Antworten, die hier richtig orthogonal geschrieben sind, gehören das zweite Beispiel von Abyx und die Antwort von Jan Hudec. Ich werde das also nicht wiederholen

if(! LessThan2Gb(file))
    return null;

if(OnNetworkDisk(file))
    return null;

(oder goto notexists;stattdessen return null;) keinen anderen Code als die hinzugefügten Zeilen beeinflussen . ZB orthogonal.

Beim Testen sollte die allgemeine Regel darin bestehen, Ausnahmen zu testen, nicht den Normalfall .

hlovdal
quelle
8
+1 für mich. Frühe Rücksendungen helfen, das Anti-Muster der Pfeilspitze zu vermeiden. Weitere Informationen finden Sie unter codinghorror.com/blog/2006/01/flattening-arrow-code.html und lostechies.com/chrismissal/2009/05/27/…. Vor dem Lesen dieses Musters habe ich immer 1 Ein- / Ausstieg pro Funktion abonniert Theorie aufgrund dessen, was mir vor etwa 15 Jahren beigebracht wurde. Ich denke, das macht den Code so viel einfacher zu lesen und, wie Sie bereits sagten, besser zu warten.
Mr Moose
3
@ MrMoose: Ihre Erwähnung der Pfeilspitze antimuster beantwortet Benjols explizite Frage: "Gibt es einen Namen für diese Art von Logik?" Poste es als Antwort und du hast meine Stimme.
outis
Dies ist eine großartige Antwort, danke. Und @MrMoose: "Pfeilspitzen-Anti-Muster" beantwortet möglicherweise meine erste Kugel, also ja, poste sie. Ich kann nicht versprechen, dass ich es annehmen werde, aber es verdient Stimmen!
Benjol
@outis. Vielen Dank. Ich habe eine Antwort hinzugefügt. Das Anti-Muster der Pfeilspitze ist auf jeden Fall in hlovdals Posten relevant, und seine Schutzklauseln funktionieren gut, wenn es darum geht, sie zu umgehen. Ich weiß nicht, wie Sie den zweiten Aufzählungspunkt dazu beantworten könnten. Ich bin nicht qualifiziert, das zu diagnostizieren :)
Mr Moose
4
+1 für "Testausnahmen, nicht der Normalfall."
Roy Tinker
25

Offensichtlich:

Whatever(Arguments)
{
    if(!FileExists(file))
        goto notexists;
    contents = OpenFile(file); // <-- prevents inclusion in if
    if(!SomeTest(contents))
        goto notexists;
    DoSomething(contents);
    return;
notexists:
    DefaultAction();
}

Sie sagten, Sie seien sogar für böse Lösungen offen. Verwenden Sie also böse Goto-Zählungen, nicht wahr?

Tatsächlich kann diese Lösung je nach Kontext weniger böse sein als entweder das zweimalige Ausführen der Aktion oder die zusätzliche Variable des Bösen. Ich habe es in eine Funktion eingewickelt, weil es in der Mitte der langen Funktion definitiv nicht OK wäre (nicht zuletzt wegen der Rückkehr in der Mitte). Aber als lange Funktion ist nicht OK, Punkt.

Wenn Sie Ausnahmen haben, sind diese leichter zu lesen, insbesondere wenn Sie OpenFile und DoSomething haben, die einfach eine Ausnahme auslösen, wenn die Bedingungen nicht erfüllt sind, so dass Sie überhaupt keine expliziten Prüfungen benötigen. In C ++ ist das Auslösen einer Ausnahme durch Java und C # hingegen eine langsame Operation, weshalb aus Performance-Gründen das Goto immer noch vorzuziehen ist.


Anmerkung zu "böse": C ++ FAQ 6.15 definiert "böse" als:

Es bedeutet, dass Sie dies und jenes die meiste Zeit vermeiden sollten , aber nicht, dass Sie es die ganze Zeit vermeiden sollten . Zum Beispiel werden Sie diese "bösen" Dinge immer dann verwenden, wenn sie "die am wenigsten bösen der bösen Alternativen" sind.

Und das gilt gotoin diesem Zusammenhang. Strukturierte Flusskontrollkonstrukte sind die meiste Zeit besser, aber wenn Sie in einer Situation sind, in der sich zu viele ihrer eigenen Übel ansammeln, wie z. B. Zustandszuweisungen, Verschachtelung von mehr als 3 Ebenen, Duplizieren von Code oder lange Zustände, gotokann dies einfach enden weniger böse zu sein.

Jan Hudec
quelle
11
Mein Mauszeiger schwebt über der Schaltfläche zum Akzeptieren ... nur um all den Puristen zu trotzen. Oooohh die Versuchung: D
Benjol 30.11.11
2
Ja ja! Dies ist die absolut "richtige" Art, Code zu schreiben. Die Struktur des Codes lautet nun "Wenn Fehler, Fehler behandeln. Normale Aktion. Wenn Fehler, Fehler behandeln. Normale Aktion", was genau so ist, wie es sein sollte. Der gesamte "normale" Code wird mit nur einer Einrückungsebene geschrieben, während der gesamte fehlerbezogene Code zwei Einrückungsebenen aufweist. So erhält der normale UND WICHTIGSTE Code den hervorstechendsten visuellen Platz und es ist möglich, den Fluss sehr schnell und einfach sequenziell nach unten abzulesen. Akzeptieren Sie diese Antwort auf jeden Fall.
hlovdal
2
Ein weiterer Aspekt ist, dass der so geschriebene Code orthogonal ist. Zum Beispiel die beiden Zeilen "if (! FileExists (file)) \ n \ tgoto notexists;" beziehen sich jetzt NUR auf die Behandlung dieses einzelnen Fehleraspekts (KISS), und am wichtigsten ist, dass keine der anderen Zeilen davon betroffen ist . In dieser Antwort unter stackoverflow.com/a/3272062/23118 sind mehrere gute Gründe aufgeführt, um den Code orthogonal zu halten.
hlovdal
5
Apropos böse Lösungen: Ich kann Ihre Lösung haben, ohne zu gehen:for(;;) { if(!FileExists(file)) break; contents = OpenFile(file); if(!SomeTest(contents)) break; DoSomething(contents); return; } /* broken out */ DefaultAction();
herby 30.11.11
4
@herby: Ihre Lösung ist böser als goto, weil Sie breakin einer Weise missbrauchen , die niemand erwartet, dass sie missbraucht wird, sodass die Leute, die den Code lesen, mehr Probleme haben, zu erkennen, wohin der Bruch sie führt, als mit goto, das es explizit sagt. Außerdem verwenden Sie eine Endlosschleife, die nur einmal ausgeführt wird, was ziemlich verwirrend ist. Leider do { ... } while(0)ist es auch nicht genau lesbar, da man sieht, dass es nur ein lustiger Block ist, wenn man am Ende ankommt und C das Brechen von anderen Blöcken nicht unterstützt (im Gegensatz zu Perl).
Jan Hudec
12
function FileContentsExists(file) {
    return FileExists(file) ? OpenFile(file) : null;
}

...

contents = FileContentExists(file);
if(contents && SomeTest(contents))
{
    DoSomething(contents);
}
else
{
    DefaultAction();
}
herby
quelle
oder gehen Sie das zusätzliche Männchen und erstellen Sie eine zusätzliche FileExistsAndConditionMet (Datei) -Methode ...
UncleZeiv
@herby SomeTestkann die gleiche Semantik wie das Vorhandensein von Dateien haben, wenn der SomeTestDateityp überprüft wird. Beispielsweise wird überprüft, ob es sich bei GIF-Dateien tatsächlich um GIF-Dateien handelt.
Abyx
1
Ja. Hängt davon ab. @ Benjol weiß es besser.
Herby
3
... natürlich meinte ich "geh die extra meile" ... :)
UncleZeiv 30.11.11
2
Das ist Ravioli zu den Extremitäten auch ich nicht gehen (und ich nehmen bin extrem in this) ... Ich denke , jetzt ist es gut lesbar ist angesichts der contents && f(contents). Zwei Funktionen, um noch eine zu speichern ?!
Herby
12

Eine Möglichkeit:

boolean handled = false;

if(FileExists(file))
{
    contents = OpenFile(file); // <-- prevents inclusion in if
    if(SomeTest(contents))
    {
        DoSomething(contents);
        handled = true;
    }
}
if (!handled)
{
    DefaultAction();
}

Dadurch wird der Code natürlich auf andere Weise etwas komplexer. Es ist also größtenteils eine Stilfrage.

Ein anderer Ansatz wäre die Verwendung von Ausnahmen, z.

try
{
    contents = OpenFile(file); // throws IO exception if file not found
    DoSomething(contents); // calls SomeTest() and throws exception on failure
}
catch(Exception e)
{
    DefaultAction();
    // and the exception should be at least logged...
}

Dies sieht einfacher aus, ist jedoch nur anwendbar, wenn

  • Wir wissen genau, welche Ausnahmen zu erwarten sind, und DefaultAction()passen zu jeder
  • Wir gehen davon aus, dass die Dateiverarbeitung erfolgreich ist und eine fehlende Datei oder ein Fehler SomeTest()eindeutig eine fehlerhafte Bedingung ist. Daher ist es angemessen, eine Ausnahme darauf zu werfen.
Péter Török
quelle
19
Noooo ~! Keine Flag-Variable, es ist definitiv falsch, weil es zu komplexen, schwer zu verstehenden (wo-wird-das-Flag-wahr) und schwer zu überarbeitenden Codes führt.
Abyx
Nicht, wenn Sie es auf einen möglichst lokalen Bereich beschränken. (function () { ... })()in Javascript, { flag = false; ... }in C-like usw.
herby
+1 für die Ausnahmelogik, die je nach Szenario die am besten geeignete Lösung sein könnte.
Steven Jeuris
4
+1 Dieses gemeinsame "Nooooo!" ist witzig. Ich denke, die Statusvariable und die vorzeitige Rückgabe sind in bestimmten Fällen beide vernünftig. Bei komplexeren Routinen würde ich mich für die Statusvariable entscheiden, da sie nicht die Komplexität erhöht, sondern die Logik explizit macht. Daran ist nichts auszusetzen.
Grossvogel
1
Dies ist unser bevorzugtes Format, in dem ich arbeite. Die 2 wichtigsten verwendbaren Optionen scheinen "multiple returns" und "flag variables" zu sein. Keiner von beiden scheint im Durchschnitt einen echten Vorteil zu haben, aber beide passen unter bestimmten Umständen besser als andere. Müssen mit Ihrem typischen Fall gehen. Nur ein weiterer "Emacs" gegen "Vi" Religionskrieg. :-)
Brian Knoblauch
11

Dies ist auf einer höheren Abstraktionsebene:

if (WeCanDoSomething(file))
{
   DoSomething(contents);
}
else
{
   DefaultAction();
} 

Und das füllt die Details aus.

boolean WeCanDoSomething(file)
{
    if FileExists(file)
    {
        contents = OpenFile(file);
        return (SomeTest(contents));
    }
    else
    {
        return FALSE;
    }
}
Jeanne Pindar
quelle
11

Funktionen sollten eine Sache tun. Sie sollten es gut machen. Sie sollten es nur tun.
- Robert Martin in Clean Code

Einige Leute finden diesen Ansatz ein wenig extrem, aber es ist auch sehr sauber. Gestatten Sie mir, in Python Folgendes zu veranschaulichen:

def processFile(self):
    if self.fileMeetsTest():
        self.doSomething()
    else:
        self.defaultAction()

def fileMeetsTest(self):
    return os.path.exists(self.path) and self.contentsTest()

def contentsTest(self):
    with open(self.path) as file:
        line = file.readline()
        return self.firstLineTest(line)

Wenn er sagt, dass Funktionen eine Sache tun sollten, bedeutet er eine Sache. processFile()wählt eine Aktion basierend auf dem Ergebnis eines Tests, und das ist alles, was es tut. fileMeetsTest()kombiniert alle Bedingungen des Tests und das ist alles, was es tut. contentsTest()überträgt die erste Zeile nach firstLineTest(), und das ist alles, was es tut.

Es scheint viele Funktionen zu haben, aber es liest sich praktisch wie normales Englisch:

Um die Datei zu verarbeiten, prüfen Sie, ob sie den Test erfüllt. Wenn ja, dann tu was. Andernfalls übernehmen Sie die Standardaktion. Die Datei besteht den Test, falls vorhanden, und besteht den Inhaltstest. Öffnen Sie zum Testen des Inhalts die Datei und testen Sie die erste Zeile. Der Test für die erste Zeile ...

Zugegeben, das ist ein bisschen wortreich, aber beachten Sie, dass ein Betreuer, der sich nicht um die Details kümmert, nach nur vier Codezeilen aufhören kann, zu lesen processFile(), und dennoch über gute Kenntnisse der Funktionsweise verfügt.

Karl Bielefeldt
quelle
5
+1 Es ist ein guter Rat, aber was "eins" ausmacht, hängt von der aktuellen Abstraktionsebene ab. processFile () ist "eins", aber zwei Dinge: fileMeetsTest () und entweder doSomething () oder defaultAction (). Ich befürchte, dass der Aspekt "eins" Anfänger verwirren könnte, die das Konzept nicht von vornherein verstehen.
Caleb
1
Es ist ein gutes Ziel ... Das ist alles, was ich dazu zu sagen habe ... ;-)
Brian Knoblauch
1
Ich mag es nicht, implizit Argumente als solche Instanzvariablen zu übergeben. Sie stecken voller "nutzloser" Instanzvariablen und es gibt viele Möglichkeiten, Ihren Zustand zu verpfuschen und die Invarianten zu brechen.
Hugomg
@Caleb, ProcessFile () macht tatsächlich eine Sache. Wie Karl in seinem Beitrag ausführt, wird anhand eines Tests entschieden, welche Aktion durchgeführt werden soll, und die tatsächliche Umsetzung der Aktionsmöglichkeiten auf andere Methoden verschoben. Wenn Sie viel mehr alternative Aktionen hinzufügen, werden die Einzelzweckkriterien für die Methode trotzdem erfüllt, solange in der unmittelbaren Methode keine logische Verschachtelung auftritt.
S.Robins
6

In Bezug auf das, was dies genannt wird, kann es sich leicht zu einem Anti-Muster der Pfeilspitze entwickeln, wenn Ihr Code wächst, um mehr Anforderungen zu erfüllen (wie aus der Antwort unter https://softwareengineering.stackexchange.com/a/122625/33922 ersichtlich ist ) gerät dann in die Falle, riesige Codeabschnitte mit verschachtelten bedingten Anweisungen zu haben, die einem Pfeil ähneln.

Siehe Links wie;

http://codinghorror.com/blog/2006/01/flattening-arrow-code.html

http://lostechies.com/chrismissal/2009/05/27/anti-patterns-and-worst-practices-the-arrowhead-anti-pattern/

Es gibt noch viel mehr zu diesem und anderen Anti-Mustern bei Google.

Einige großartige Tipps, die Jeff in seinem Blog dazu gibt, sind:

1) Ersetzen Sie die Bedingungen durch Schutzklauseln.

2) Zerlegen Sie bedingte Blöcke in separate Funktionen.

3) Negative Schecks in positive Schecks umwandeln

4) Immer so bald wie möglich opportunistisch von der Funktion zurückkehren.

Lesen Sie auch einige Kommentare in Jeffs Blog zu Steve McConnells Vorschlägen zu frühen Retouren.

"Verwenden Sie einen Return, wenn er die Lesbarkeit verbessert: Wenn Sie in bestimmten Routinen die Antwort kennen, möchten Sie sie sofort an die aufrufende Routine zurückgeben. Wenn die Routine so definiert ist, dass sie keine weitere Bereinigung erfordert Wenn ein Fehler erkannt wird und nicht sofort zurückgegeben wird, müssen Sie mehr Code schreiben. "

...

"Minimieren Sie die Anzahl der Retouren in jeder Routine: Es ist schwieriger, eine Routine zu verstehen, wenn Sie sie unten lesen und sich der Möglichkeit nicht bewusst sind, dass sie irgendwo anders zurückkehrt. Verwenden Sie Retouren daher mit Bedacht - nur, wenn sie sich verbessern Lesbarkeit. "

Aufgrund dessen, was mir vor ungefähr 15 Jahren beigebracht wurde, habe ich immer 1 Ein- / Ausstieg pro Funktionstheorie abonniert. Ich denke, das macht den Code so viel einfacher zu lesen und, wie Sie bereits sagten, besser zu warten

Mr Moose
quelle
6

Dies entspricht den DRY-, no-goto- und no-multiple-return-Regeln und ist meiner Meinung nach skalierbar und lesbar:

success = FileExists(file);
if (success)
{
    contents = OpenFile(file);
    success = SomeTest(contents);
}
if (success)
{
    DoSomething(contents);
}
else
{
    DefaultAction();
}
mouviciel
quelle
1
Standardkonformität ist jedoch nicht unbedingt gleichbedeutend mit gutem Code. Ich bin derzeit unentschlossen über diesen Codeausschnitt.
Brian Knoblauch
dies ersetzt nur 2 defaultAction (); mit 2 identischen if Bedingungen und fügt eine Flagvariable hinzu, die imo viel schlechter ist.
Ryathal
3
Der Vorteil eines solchen Konstrukts besteht darin, dass der Code mit zunehmender Anzahl von Tests nicht mehr ifs in anderen ifs verschachtelt . Außerdem befindet sich der Code zur Behandlung des erfolglosen Falls ( DefaultAction()) nur an einer Stelle, und zu Debugging-Zwecken springt der Code nicht um Hilfsfunktionen herum, und das Hinzufügen von Haltepunkten zu den Zeilen, in denen die successVariable geändert wird, kann schnell anzeigen, welche Tests bestanden wurden (oberhalb der ausgelösten) Haltepunkt) und welche nicht getestet wurden (siehe unten).
Frozenkoi
1
Yeeaah, es gefällt mir irgendwie, aber ich glaube, ich würde umbenennen successin ok_so_far:)
Benjol
Dies ist sehr ähnlich zu dem, was ich mache, wenn (1) der Prozess sehr linear ist, wenn alles richtig läuft und (2) Sie ansonsten die Pfeil-Anti-Muster haben würden. Ich versuche jedoch, das Hinzufügen einer zusätzlichen Variablen zu vermeiden, was normalerweise einfach ist, wenn Sie die Voraussetzungen für den nächsten Schritt berücksichtigen (was sich geringfügig von der Frage unterscheidet, ob ein früherer Schritt fehlgeschlagen ist). Wenn die Datei vorhanden ist, öffnen Sie die Datei. Wenn die Datei geöffnet ist, lesen Sie den Inhalt. Wenn ich Inhalte habe, verarbeite sie, sonst mache ich die Standardaktion.
Adrian McCarthy
3

Ich würde es auf eine separate Methode extrahieren und dann:

if(!FileExists(file))
{
    DefaultAction();
    return;
}

contents = OpenFile(file);
if(!SomeTest(contents))
{
    DefaultAction();
    return;
}

DoSomething(contents);

was auch erlaubt

if(!FileExists(file))
{
    DefaultAction();
    return Result.FileNotFound;
}

contents = OpenFile(file);
if(!SomeTest(contents))
{
    DefaultAction();
    return Result.TestFailed;
}

DoSomething(contents);
return Result.Success;            

dann könntest du evtl. die anrufe entfernen DefaultActionund die DefaultActionfür den anrufer ausführen lassen :

Result OurMethod(file)
{
    if(!FileExists(file))
    {
        return Result.FileNotFound;
    }

    contents = OpenFile(file);
    if(!SomeTest(contents))
    {
        return Result.TestFailed;
    }

    DoSomething(contents);
    return Result.Success;            
}

void Caller()
{
    // something, something...

    var result = OurMethod(file);
    // if (result == Result.FileNotFound || result == Result.TestFailed), or just
    if (result != Result.Success)        
    {
        DefaultAction();
    }
}

Mir gefällt auch Jeanne Pindars Ansatz .

Konrad Morawski
quelle
3

Für diesen speziellen Fall ist die Antwort einfach genug ...

Zwischen FileExistsund besteht eine Wettlaufsituation OpenFile: Was passiert, wenn die Datei entfernt wird?

Der einzig vernünftige Weg, mit diesem speziellen Fall umzugehen, besteht darin, Folgendes zu überspringen FileExists:

contents = OpenFile(file);
if (!contents) // open failed
    DefaultAction();
else (SomeTest(contents))
    DoSomething(contents);

Dies löst dieses Problem ordentlich und macht den Code sauberer.

Im Allgemeinen: Versuchen Sie, das Problem zu überdenken und eine andere Lösung zu finden, die das Problem vollständig umgeht.

Martin Wickman
quelle
2

Eine andere Möglichkeit, wenn Sie nicht zu viele der anderen sehen möchten, besteht darin, die Verwendung von else ganz fallen zu lassen und eine zusätzliche return-Anweisung einzugeben. Andernfalls ist dies überflüssig, es sei denn, Sie benötigen eine komplexere Logik, um festzustellen, ob es mehr als nur zwei Aktionsmöglichkeiten gibt.

So könnte Ihr Beispiel lauten:

void DoABunchOfStuff()
{
    if(FileExists(file))
    {
        DoSomethingWithFileContent(file);
        return;
    }

    DefaultAction();
}

void DoSomethingWithFileContent(file)
{        
    var contents = GetFileContents(file)

    if(SomeTest(contents))
    {
        DoSomething(contents);
        return;
    }

    DefaultAction();
}

AReturnType GetFileContents(file)
{
    return OpenFile(file);
}

Persönlich macht es mir nichts aus, die else- Klausel zu verwenden, da sie explizit angibt, wie die Logik funktionieren soll, und so die Lesbarkeit Ihres Codes verbessert. Einige Code-Verschönerungstools ziehen es jedoch vor, die Verschachtelungslogik auf eine einzige if- Anweisung zu vereinfachen .

S.Robins
quelle
2

Der im Beispielcode gezeigte Fall kann normalerweise auf eine einzelne ifAnweisung reduziert werden . Auf vielen Systemen gibt die Funktion zum Öffnen von Dateien einen ungültigen Wert zurück, wenn die Datei noch nicht vorhanden ist. Manchmal ist dies das Standardverhalten. In anderen Fällen muss dies über ein Argument angegeben werden. Dies bedeutet, dass der FileExistsTest fallengelassen werden kann, was auch bei Rennbedingungen hilfreich sein kann, die sich aus dem Löschen der Datei zwischen dem Existenztest und dem Öffnen der Datei ergeben.

file = OpenFile(path);
if(isValidFileHandle(file) && SomeTest(file)) {
    DoSomething(file);
} else {
    DefaultAction();
}

Hiermit wird das Problem des Mischens von Abstraktionsebenen nicht direkt behoben, da es das Problem mehrerer, nicht verkettbarer Tests gänzlich umgeht, obwohl das Aufheben des Tests der Dateiexistenz nicht mit dem Trennen von Abstraktionsebenen unvereinbar ist. Angenommen, ungültige Dateihandles entsprechen "false" und Dateihandles werden geschlossen, wenn sie den Gültigkeitsbereich verlassen:

OpenFileIfSomething(path:String) : FileHandle {
    file = OpenFile(path);
    if (file && SomeTest(file)) {
        return file;
    }
    return null;
}

...

if ((file = OpenFileIfSomething(path))) {
    DoSomething(file);
} else {
    DefaultAction();
}
outis
quelle
2

Ich bin mit frozenkoi einverstanden, aber für C # dachte ich trotzdem, es würde helfen, der Syntax der TryParse-Methoden zu folgen.

if(FileExists(file) && TryOpenFile(file, out contents))
    DoSomething(contents);
else
    DefaultAction();
bool TryOpenFile(object file, out object contents)
{
    try{
        contents = OpenFile(file);
    }
    catch{
        //something bad happened, computer probably exploded
        return false;
    }
    return true;
}
Biff MaGriff
quelle
1

Ihr Code ist hässlich, weil Sie in einer einzelnen Funktion zu viel tun. Sie möchten die Datei entweder verarbeiten oder die Standardaktion ausführen. Sagen Sie zunächst Folgendes:

if (!ProcessFile(file)) { 
  DefaultAction(); 
}

Perl- und Ruby-Programmierer schreiben processFile(file) || defaultAction()

Nun schreibe ProcessFile:

if (FileExists(file)) { 
  contents = OpenFile(file);
  if (SomeTest(contents)) {
    processContents(contents);
    return true;
  }
}
return false;
Kevin Cline
quelle
1

Natürlich können Sie in solchen Szenarien nur so weit gehen, aber hier ist eine Möglichkeit:

interface File<T> {
    function isOK():Bool;
    function getData():T;
}

var appleFile:File<Apple> = appleStorage.get(fileURI);
if (appleFile.isOK())
    eat(file.getData());
else
    cry();

Möglicherweise möchten Sie zusätzliche Filter. Dann mach das:

var appleFile = appleStorage.get(fileURI, isEdible);
//isEdible is of type Apple->Bool and will be used internally to answer to the isOK call
if (appleFile.isOK())
    eat(file.getData());
else
    cry();

Obwohl dies genauso gut Sinn machen könnte:

function eat(apple:Apple) {
     if (isEdible(apple)) 
         digest(apple);
     else
         die();
}
var appleFile = appleStorage.get(fileURI);
if (appleFile.isOK())
    eat(appleFile.getData());
else
    cry();

Welches ist das Beste? Das hängt vom realen Problem ab, mit dem Sie konfrontiert sind.
Aber das, was man mitnehmen sollte, ist: Mit Komposition und Polymorphismus kann man viel anfangen.

back2dos
quelle
1

Was ist los mit dem Offensichtlichen?

if(!FileExists(file)) {
    DefaultAction();
    return;
}
contents = OpenFile(file);
if(!SomeTest(contents))
{
    DefaultAction();
    return;
}        
DoSomething(contents);

Es scheint mir ziemlich normal zu sein? Für eine solche große Prozedur, bei der viele kleine Dinge passieren müssen, deren Fehlschlagen letztere verhindern würde. Ausnahmen machen es ein bisschen sauberer, wenn das eine Option ist.

Steve Bennett
quelle
0

Mir ist klar, dass dies eine alte Frage ist, aber ich habe ein Muster bemerkt, das nicht erwähnt wurde. hauptsächlich das Setzen einer Variablen, um später die Methode (n) zu bestimmen, die Sie aufrufen möchten (außerhalb des if ... else ...).

Dies ist nur ein weiterer Blickwinkel, um die Arbeit mit dem Code zu vereinfachen. Außerdem können Sie festlegen, wann eine andere aufzurufende Methode hinzugefügt oder die entsprechende Methode geändert werden soll, die in bestimmten Situationen aufgerufen werden muss.

Anstatt alle Erwähnungen der Methode (und möglicherweise einige fehlende Szenarien) ersetzen zu müssen, werden sie alle am Ende des if ... else ... -Blocks aufgelistet und sind einfacher zu lesen und zu ändern. Ich neige dazu, dies zu verwenden, wenn zum Beispiel mehrere Methoden aufgerufen werden, aber innerhalb des verschachtelten if ... else ... kann eine Methode in mehreren Übereinstimmungen aufgerufen werden.

Wenn Sie eine Variable festlegen, die den Status definiert, haben Sie möglicherweise viele tief verschachtelte Optionen und können den Status aktualisieren, wenn etwas ausgeführt werden soll (oder nicht).

Dies könnte wie im Beispiel in der Frage verwendet werden, in der überprüft wird, ob 'DoSomething' aufgetreten ist, und wenn nicht, führen Sie die Standardaktion aus. Oder Sie können für jede Methode, die Sie aufrufen möchten, einen Status festlegen und die entsprechende Methode außerhalb von if ... else ... aufrufen.

Am Ende der verschachtelten if ... else ... -Anweisungen überprüfen Sie den Status und handeln entsprechend. Dies bedeutet, dass Sie nur eine einzige Erwähnung einer Methode anstelle aller Stellen benötigen, an denen sie angewendet werden soll.

bool ActionDone = false;

if (Method_1(object_A)) // Test 1
{
    result_A = Method_2(object_A); // Result 1

    if (Method_3(result_A)) // Test 2
    {
        Method_4(result_A); // Action 1
        ActionDone = true;
    }
}

if (!ActionDone)
{
    Method_5(); // Default Action
}
Steve Padmore
quelle
0

So reduzieren Sie verschachtelte IF:

1 / vorzeitige Rückkehr;

2 / zusammengesetzter Ausdruck (kurzschlussbewusst)

So könnte Ihr Beispiel wie folgt überarbeitet werden:

if( FileExists(file) && SomeTest(contents = OpenFile(file)) )
{
    DoSomething(contents);
    return;
}
DefaultAction();
DQ_vn
quelle
0

Ich habe viele Beispiele mit "return" gesehen, die ich auch verwende, aber manchmal möchte ich das Erstellen neuer Funktionen vermeiden und stattdessen eine Schleife verwenden:

while (1) {
    if (FileExists(file)) {
        contents = OpenFile(file);
        if (SomeTest(contents)) {
           DoSomething(contents);
           break;
        } 
    }
    DefaultAction();
    break;
}

Wenn Sie weniger Zeilen schreiben möchten oder Endlosschleifen hassen, können Sie den Schleifentyp in "do ... while (0)" ändern und den letzten "break" vermeiden.

XzKto
quelle
0

Wie wäre es mit dieser Lösung:

content = NULL; //I presume OpenFile returns a pointer 
if(FileExists(file))
    contents = OpenFile(file);
if(content != NULL && SomeTest(contents))
    DoSomething(contents);
else
    DefaultAction();

Ich bin davon ausgegangen, dass OpenFile einen Zeiger zurückgibt, aber dies könnte auch mit dem Werttyp return funktionieren, indem ein Standardwert angegeben wird, der nicht zurückgegeben werden kann (Fehlercodes oder ähnliches).

Natürlich erwarte ich keine mögliche Aktion über die SomeTest-Methode für den NULL-Zeiger (aber Sie wissen es nie), so dass dies auch als zusätzliche Überprüfung für den NULL-Zeiger für den SomeTest-Aufruf (Inhalt) angesehen werden kann.

chedi
quelle
0

Die eleganteste und prägnanteste Lösung ist natürlich die Verwendung eines Präprozessor-Makros.

#define DOUBLE_ELSE(CODE) else { CODE } } else { CODE }

Womit Sie schönen Code wie diesen schreiben können:

if(FileExists(file))
{
    contents = OpenFile(file);
    if(SomeTest(contents))
    {
        DoSomething(contents);
    }
    DOUBLE_ELSE(DefaultAction();)

Wenn Sie diese Technik häufig verwenden, ist es möglicherweise schwierig, sich auf die automatische Formatierung zu verlassen, und einige IDEs schreien Sie möglicherweise ein wenig darüber an, was fälschlicherweise als fehlerhaft angenommen wird. Und wie das Sprichwort sagt, ist alles ein Kompromiss, aber ich nehme an, es ist kein schlechter Preis, um die Übel von wiederholtem Code zu vermeiden.

Peter Olson
quelle
Für einige Menschen, und in einigen Sprachen Präprozessormakros sind böse Code :)
Benjol
@Benjol Sie sagten, Sie seien offen für böse Vorschläge, nein? ;)
Peter Olson
Ja, absolut, es war nur für Sie "Vermeiden Sie die Übel" :)
Benjol
4
Das ist so schrecklich, ich musste es nur verbessern: D
back2dos
Shirley, das meinst du nicht ernst !!!!!!
Jim In Texas
-1

Da Sie aus Neugier gefragt haben und Ihre Frage nicht mit einer bestimmten Sprache markiert ist (obwohl klar ist, dass Sie zwingende Sprachen im Sinn hatten), ist es möglicherweise sinnvoll hinzuzufügen, dass Sprachen, die eine verzögerte Bewertung unterstützen, einen völlig anderen Ansatz ermöglichen. In diesen Sprachen werden Ausdrücke nur bei Bedarf ausgewertet, sodass Sie "Variablen" definieren und sie nur verwenden können, wenn dies sinnvoll ist. In einer fiktiven Sprache mit Lazy let/ inStrukturen vergessen Sie beispielsweise die Flusskontrolle und schreiben:

let
  contents = ReadFile(file)
in
  if FileExists(file) && SomeTest(contents) 
    DoSomething(contents)
  else 
    DefaultAction()
tokland
quelle