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 ...
coding-style
conditions
Benjol
quelle
quelle
DefaultAction
Anrufe verletzen das DRY-Prinzipmake 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.Antworten:
Extrahieren Sie es, um die Funktion (Methode) und die
return
Anweisung use zu trennen :Oder, vielleicht besser, trennen Sie das Abrufen von Inhalten und deren Verarbeitung:
Update:
Warum keine Ausnahmen, warum
OpenFile
keineE / A- Ausnahme: Ich denke, es handelt sich eher um eine allgemeine Frage als um eine Frage zu Datei-E / A. Namen wie
FileExists
,OpenFile
können verwirrend sein , aber wenn sie mit zu ersetzenFoo
,Bar
usw. - es wäre klarer , dassDefaultAction
wie so oft genannt werden kannDoSomething
, so dass es nicht-Ausnahmefall sein kann. Péter Török schrieb am Ende seiner Antwort darüberWarum es in der 2. Variante einen ternären bedingten Operator
gibt : Wenn es ein [C ++] - Tag geben würde, würde ich eine
if
Anweisung mit der Deklaration voncontents
in ihrem Bedingungsteil schreiben: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 dieget_contents
Funktion war, habe ich nur die kürzere Version verwendet (und auchcontents
Typ weggelassen ). Jedenfalls steht es außer Frage.quelle
Wenn die von Ihnen verwendete Programmiersprache (0) binäre Vergleiche kurzschließt (dh wenn nicht aufgerufen wird,
SomeTest
wennFileExists
falsch zurückgegeben wird) und (1) Zuweisung einen Wert zurückgibt (das Ergebnis vonOpenFile
wird zugewiesen,contents
und dieser Wert wird dann als Argument übergeben BisSomeTest
) 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.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
DefaultAction
in diesem Fall den Fehler behandelt ).quelle
if
Meiner Meinung nach ist es ziemlich eklig, so viel Code in eine Aussage zu setzen .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:
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:
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
(oder
goto notexists;
stattdessenreturn 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 .
quelle
Offensichtlich:
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:
Und das gilt
goto
in 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,goto
kann dies einfach enden weniger böse zu sein.quelle
for(;;) { if(!FileExists(file)) break; contents = OpenFile(file); if(!SomeTest(contents)) break; DoSomething(contents); return; } /* broken out */ DefaultAction();
goto
, weil Siebreak
in 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. Leiderdo { ... } 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)....
quelle
SomeTest
kann die gleiche Semantik wie das Vorhandensein von Dateien haben, wenn derSomeTest
Dateityp überprüft wird. Beispielsweise wird überprüft, ob es sich bei GIF-Dateien tatsächlich um GIF-Dateien handelt.contents && f(contents)
. Zwei Funktionen, um noch eine zu speichern ?!Eine Möglichkeit:
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.
Dies sieht einfacher aus, ist jedoch nur anwendbar, wenn
DefaultAction()
passen zu jederSomeTest()
eindeutig eine fehlerhafte Bedingung ist. Daher ist es angemessen, eine Ausnahme darauf zu werfen.quelle
(function () { ... })()
in Javascript,{ flag = false; ... }
in C-like usw.Dies ist auf einer höheren Abstraktionsebene:
Und das füllt die Details aus.
quelle
Einige Leute finden diesen Ansatz ein wenig extrem, aber es ist auch sehr sauber. Gestatten Sie mir, in Python Folgendes zu veranschaulichen:
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 nachfirstLineTest()
, und das ist alles, was es tut.Es scheint viele Funktionen zu haben, aber es liest sich praktisch wie normales Englisch:
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.quelle
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:
Lesen Sie auch einige Kommentare in Jeffs Blog zu Steve McConnells Vorschlägen zu frühen Retouren.
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
quelle
Dies entspricht den DRY-, no-goto- und no-multiple-return-Regeln und ist meiner Meinung nach skalierbar und lesbar:
quelle
if
s in anderenif
s 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 diesuccess
Variable geändert wird, kann schnell anzeigen, welche Tests bestanden wurden (oberhalb der ausgelösten) Haltepunkt) und welche nicht getestet wurden (siehe unten).success
inok_so_far
:)Ich würde es auf eine separate Methode extrahieren und dann:
was auch erlaubt
dann könntest du evtl. die anrufe entfernen
DefaultAction
und dieDefaultAction
für den anrufer ausführen lassen :Mir gefällt auch Jeanne Pindars Ansatz .
quelle
Für diesen speziellen Fall ist die Antwort einfach genug ...
Zwischen
FileExists
und besteht eine WettlaufsituationOpenFile
: Was passiert, wenn die Datei entfernt wird?Der einzig vernünftige Weg, mit diesem speziellen Fall umzugehen, besteht darin, Folgendes zu überspringen
FileExists
: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.
quelle
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:
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 .
quelle
Der im Beispielcode gezeigte Fall kann normalerweise auf eine einzelne
if
Anweisung 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 derFileExists
Test 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.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:
quelle
Ich bin mit frozenkoi einverstanden, aber für C # dachte ich trotzdem, es würde helfen, der Syntax der TryParse-Methoden zu folgen.
quelle
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:
Perl- und Ruby-Programmierer schreiben
processFile(file) || defaultAction()
Nun schreibe ProcessFile:
quelle
Natürlich können Sie in solchen Szenarien nur so weit gehen, aber hier ist eine Möglichkeit:
Möglicherweise möchten Sie zusätzliche Filter. Dann mach das:
Obwohl dies genauso gut Sinn machen könnte:
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.
quelle
Was ist los mit dem Offensichtlichen?
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.
quelle
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.
quelle
So reduzieren Sie verschachtelte IF:
1 / vorzeitige Rückkehr;
2 / zusammengesetzter Ausdruck (kurzschlussbewusst)
So könnte Ihr Beispiel wie folgt überarbeitet werden:
quelle
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:
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.
quelle
Wie wäre es mit dieser Lösung:
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.
quelle
Die eleganteste und prägnanteste Lösung ist natürlich die Verwendung eines Präprozessor-Makros.
Womit Sie schönen Code wie diesen schreiben können:
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.
quelle
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
/in
Strukturen vergessen Sie beispielsweise die Flusskontrolle und schreiben:quelle