Ist es falsch, einen booleschen Parameter zu verwenden, um das Verhalten zu bestimmen?

194

Ich habe von Zeit zu Zeit eine Übung gesehen, die sich "falsch" anfühlt, aber ich kann nicht genau artikulieren, was daran falsch ist. Oder vielleicht ist es nur mein Vorurteil. Hier geht:

Ein Entwickler definiert eine Methode mit einem Booleschen Wert als einen ihrer Parameter, und diese Methode ruft einen anderen auf. Dieser Boolesche Wert wird schließlich nur verwendet, um zu bestimmen, ob eine bestimmte Aktion ausgeführt werden soll oder nicht. Dies kann zum Beispiel verwendet werden, um die Aktion nur dann zuzulassen, wenn der Benutzer bestimmte Rechte besitzt oder wenn wir uns im Testmodus, Batch-Modus oder Live-Modus befinden (oder nicht) oder wenn sich das System in einem befindet bestimmter Zustand.

Nun, es gibt immer eine andere Möglichkeit, dies zu tun, sei es durch Abfragen, wann es Zeit ist, die Aktion auszuführen (anstatt den Parameter zu übergeben), oder durch mehrere Versionen der Methode oder mehrere Implementierungen der Klasse usw. Meine Frage ist nicht Es geht nicht so sehr darum, dies zu verbessern, sondern vielmehr darum, ob es wirklich falsch ist (wie ich vermute), und wenn ja, was ist daran falsch?

Strahl
quelle
1
Hier geht es darum, wohin Entscheidungen gehören. Bewegen Sie die Entscheidungen an einen zentralen Ort, anstatt sie überall liegen zu lassen. Dadurch bleibt die Komplexität geringer als wenn Sie jedes Mal einen Faktor zwei der Codepfade haben.
28
Martin Fowler hat einen Artikel darüber: martinfowler.com/bliki/FlagArgument.html
Christoffer Hammarström
@ ChristofferHammarström Netter Link. Ich werde das in meine Antwort aufnehmen, da es in vielen Details die gleiche Idee meiner Erklärung erklärt.
Alex
1
Ich stimme nicht immer mit dem überein, was Nick zu sagen hat, aber in diesem Fall stimme ich zu 100% zu: Verwenden Sie keine booleschen Parameter .
Marjan Venema

Antworten:

107

Ja, dies ist wahrscheinlich ein Codegeruch, der zu nicht wartbarem Code führen würde, der schwer zu verstehen ist und dessen Wahrscheinlichkeit geringer ist, leicht wiederverwendet zu werden.

Wie andere Plakate bereits festgestellt haben, ist der Kontext alles (gehen Sie nicht zu grob, wenn er einmalig ist oder wenn die Praxis als absichtlich entstandene technische Verschuldung eingestuft wurde, die später überarbeitet werden soll), sondern im Großen und Ganzen, wenn ein Parameter übergeben wird in eine Funktion, die ein bestimmtes auszuführendes Verhalten auswählt, ist eine weitere schrittweise Verfeinerung erforderlich; Wenn Sie diese Funktion in kleinere Funktionen aufteilen, erhalten Sie stärker zusammenhängende Funktionen.

Was ist also eine sehr zusammenhängende Funktion?

Es ist eine Funktion, die nur eine Sache und eine Sache tut.

Das Problem mit einem Parameter, der wie beschrieben übergeben wird, besteht darin, dass die Funktion mehr als zwei Dinge ausführt. Abhängig vom Status des Booleschen Parameters kann er die Zugriffsrechte des Benutzers überprüfen oder nicht. Abhängig von diesem Entscheidungsbaum führt er eine Funktionalität aus.

Es wäre besser, die Belange der Zugriffskontrolle von den Belangen der Aufgabe, Aktion oder des Befehls zu trennen.

Wie Sie bereits bemerkt haben, scheint die Verflechtung dieser Bedenken nicht in Ordnung zu sein.

Der Begriff der Kohäsivität hilft uns zu identifizieren, dass die fragliche Funktion nicht sehr kohäsiv ist und dass wir den Code umgestalten könnten, um eine Reihe von kohäsiveren Funktionen zu erzeugen.

Die Frage könnte also erneut gestellt werden. Angesichts der Tatsache, dass wir uns alle einig sind, dass die Übergabe von Verhaltensauswahlparametern am besten vermieden wird, wie können wir die Situation verbessern?

Ich würde den Parameter komplett loswerden. Die Möglichkeit, die Zugriffskontrolle auch für Testzwecke auszuschalten, ist ein potenzielles Sicherheitsrisiko. Zu Testzwecken können Sie die Zugriffsprüfung entweder oder , um sowohl das Szenario "Zugriff erlaubt" als auch das Szenario "Zugriff verweigert" zu testen.

Ref: Kohäsion (Informatik)

rauben
quelle
Rob, würdest du erklären, was Kohäsion ist und wie sie angewendet wird?
Ray
Ray, je mehr ich darüber nachdenke, desto mehr denke ich, dass Sie den Code ablehnen sollten, der auf der Sicherheitslücke basiert, die der Boolesche Wert zum Deaktivieren der Zugriffskontrolle in die Anwendung einführt. Die Qualitätsverbesserung der Codebasis wird ein netter Nebeneffekt sein;)
Rob
1
Sehr schöne Erklärung des Zusammenhalts und seiner Anwendung. Dies sollte wirklich mehr Stimmen bekommen. Und ich stimme auch mit dem Sicherheitsproblem überein. Wenn es sich jedoch nur um private Methoden handelt, handelt es sich um eine kleinere potenzielle Sicherheitsanfälligkeit
Ray,
1
Vielen Dank, Ray. Hört sich so an, als wäre es einfach genug, die Faktoren zu überdenken, wenn es die Zeit erlaubt. Es könnte sich lohnen, einen TODO-Kommentar einzutragen, um das Problem hervorzuheben und ein Gleichgewicht zwischen Fachkompetenz und Sensibilität für den Druck zu finden, unter dem wir manchmal stehen, um Dinge zu erledigen.
Rob
"unmaintainable"
aaa90210
149

Ich habe dieses Muster vor langer Zeit aus einem sehr einfachen Grund nicht mehr verwendet. Wartungskosten. Mehrmals stellte ich fest, dass ich eine Funktion hatte, frobnicate(something, forwards_flag)die mehrfach in meinem Code aufgerufen wurde, und alle Stellen im Code suchen musste, an denen der Wert falseals Wert von übergeben wurde forwards_flag. Sie können nicht einfach danach suchen, was zu Wartungsproblemen führt. Und wenn Sie an jeder dieser Sites einen Bugfix vornehmen müssen, kann es sein, dass Sie ein unglückliches Problem haben, wenn Sie eines verpassen.

Dieses spezifische Problem lässt sich jedoch leicht beheben, ohne den Ansatz grundlegend zu ändern:

enum FrobnicationDirection {
  FrobnicateForwards,
  FrobnicateBackwards;
};

void frobnicate(Object what, FrobnicationDirection direction);

Mit diesem Code müsste man nur nach Instanzen von suchen FrobnicateBackwards. Es ist zwar möglich, dass es einen Code gibt, der dies einer Variablen zuweist, so dass Sie ein paar Steuerthreads folgen müssen. In der Praxis ist dies jedoch so selten, dass diese Alternative in Ordnung ist.

Es gibt jedoch ein anderes Problem, die Flagge zumindest im Prinzip auf diese Weise zu übergeben. Dies liegt daran, dass einige (nur einige) Systeme mit diesem Design möglicherweise zu viel Wissen über die Implementierungsdetails der tief verschachtelten Teile des Codes (der das Flag verwendet) an die äußeren Schichten weitergeben (die wissen müssen, welcher Wert übergeben werden soll) in dieser Flagge). Um die Terminologie von Larry Constantine zu verwenden, besteht möglicherweise eine zu starke Kopplung zwischen dem Setter und dem Benutzer der Booleschen Flagge. Franky, obwohl es schwierig ist, diese Frage mit Sicherheit zu beantworten, ohne mehr über die Codebasis zu wissen.

Um auf die konkreten Beispiele einzugehen, die Sie nennen, hätte ich in jeder Hinsicht einige Bedenken, jedoch hauptsächlich aus Gründen des Risikos / der Richtigkeit. Das heißt, wenn Ihr System Flags weitergeben muss, die angeben, in welchem ​​Zustand sich das System befindet, stellen Sie möglicherweise fest, dass Sie Code haben, der dies hätte berücksichtigen sollen, den Parameter jedoch nicht überprüft (da er nicht an übergeben wurde) diese Funktion). Sie haben also einen Fehler, weil jemand den Parameter nicht übergeben hat.

Man muss auch zugeben, dass ein Systemstatusindikator, der an fast jede Funktion übergeben werden muss, im Wesentlichen eine globale Variable ist. Viele der Nachteile einer globalen Variablen werden zutreffen. Ich denke, in vielen Fällen ist es besser, die Kenntnis des Systemzustands (oder der Anmeldeinformationen des Benutzers oder der Systemidentität) in einem Objekt zu kapseln, das dafür verantwortlich ist, auf der Grundlage dieser Daten korrekt zu handeln. Dann übergeben Sie einen Verweis auf dieses Objekt im Gegensatz zu den Rohdaten. Das Schlüsselkonzept ist hier die Kapselung .

James Youngman
quelle
9
Wirklich großartige konkrete Beispiele sowie Einblicke in die Natur dessen, womit wir es zu tun haben und wie es uns beeinflusst.
Ray
33
+1. Ich benutze dafür so oft wie möglich Aufzählungen. Ich habe Funktionen gesehen, bei denen später zusätzliche boolParameter hinzugefügt wurden und die Aufrufe so aussehen DoSomething( myObject, false, false, true, false ). Es ist unmöglich herauszufinden, was die zusätzlichen booleschen Argumente bedeuten, während es mit sinnvoll benannten Enum-Werten einfach ist.
Graeme Perrow
17
Oh man, endlich ein gutes Beispiel dafür, wie man Backwards frobniziert. Ich habe für immer danach gesucht.
Alex Pritchard
38

Dies ist nicht unbedingt falsch, kann aber einen Codegeruch darstellen .

Das grundlegende Szenario, das in Bezug auf boolesche Parameter vermieden werden sollte, ist:

public void foo(boolean flag) {
    doThis();
    if (flag)
        doThat();
}

Wenn Sie dann anrufen, rufen Sie normalerweise an foo(false)und das foo(true)hängt von dem genauen Verhalten ab, das Sie möchten.

Dies ist wirklich ein Problem, da es sich um einen schlechten Zusammenhalt handelt. Sie erstellen eine Abhängigkeit zwischen Methoden, die nicht wirklich erforderlich ist.

Was Sie in diesem Fall tun sollten, ist verlassen doThisund doThatals separate und öffentliche Methoden dann tun:

doThis();
doThat();

oder

doThis();

Auf diese Weise überlassen Sie die richtige Entscheidung dem Aufrufer (genau so, als würden Sie einen booleschen Parameter übergeben), ohne eine Kopplung zu erstellen.

Natürlich werden nicht alle booleschen Parameter so schlecht verwendet, aber es ist definitiv ein Codegeruch und Sie haben Recht, misstrauisch zu werden, wenn Sie das viel im Quellcode sehen.

Dies ist nur ein Beispiel für die Lösung dieses Problems anhand der von mir geschriebenen Beispiele. In anderen Fällen ist ein anderer Ansatz erforderlich.

Es gibt einen guten Artikel von Martin Fowler , der die gleiche Idee ausführlicher erklärt.

PS: Wenn Methode fooanstelle von zwei einfachen Methoden eine komplexere Implementierung hatte, müssen Sie nur eine kleine Refactoring- Extraktionsmethode anwenden , damit der resultierende Code der von foomir geschriebenen Implementierung ähnelt .

Alex
quelle
1
Vielen Dank, dass Sie den Begriff "Code-Geruch" ausgesprochen haben. Ich wusste, dass es schlecht roch, konnte aber den Geruch nicht richtig erkennen. Ihr Beispiel passt ziemlich genau zu dem, was ich ansehe.
Ray
24
Es gibt viele Situationen, in denen das if (flag) doThat()Innere foo()legitim ist. doThat()Wenn Sie die Entscheidung über das Aufrufen an jeden Aufrufer weitergeben, wird die Wiederholung erzwungen, die entfernt werden muss, wenn Sie später herausfinden, welche Methoden verwendet werden, und das flagVerhalten muss ebenfalls aufgerufen werden doTheOther(). Ich würde viel lieber Abhängigkeiten zwischen Methoden in dieselbe Klasse setzen, als später alle Aufrufer durchsuchen zu müssen.
Blrfl
1
@Blrfl: Ja, ich denke, die einfacheren Refactorings wären entweder das Erstellen von a doOneund doBoth-Methoden (für den falschen bzw. den wahren Fall) oder die Verwendung eines separaten Aufzählungstyps, wie von James Youngman vorgeschlagen
hugomg
@missingno: Sie hätten immer noch das gleiche Problem, redundanten Code an Anrufer weiterzuleiten, um die doOne()oder doBoth()-Entscheidung zu treffen. Unterprogramme / Funktionen / Methoden haben Argumente, so dass ihr Verhalten variiert werden kann. Die Verwendung einer Aufzählung für eine wirklich boolesche Bedingung klingt nach einer Wiederholung, wenn der Name des Arguments bereits erklärt, was es bewirkt.
Blrfl
3
Wenn der Aufruf von zwei Methoden nacheinander oder nur einer Methode als redundant betrachtet werden kann, ist es auch redundant, wie Sie einen Try-Catch-Block oder ein If-else schreiben. Bedeutet das, dass Sie eine Funktion schreiben, um alle zusammenzufassen? Nein! Seien Sie vorsichtig, wenn Sie eine Methode erstellen, die nur zwei andere Methoden aufruft, ist dies nicht unbedingt eine gute Abstraktion.
Alex
29

Zunächst einmal: Programmieren ist weniger eine Wissenschaft als vielmehr eine Kunst. Es gibt also sehr selten einen "falschen" und einen "richtigen" Weg, um zu programmieren. Die meisten Codierungsstandards sind lediglich "Einstellungen", die einige Programmierer für nützlich halten. aber letztendlich sind sie eher willkürlich. Daher würde ich eine Parameterauswahl niemals als an und für sich "falsch" bezeichnen - und schon gar nicht als etwas, das so allgemein und nützlich ist wie ein boolescher Parameter. Die Verwendung eines boolean(oder eines int) zur Verkapselung von Zuständen ist in vielen Fällen durchaus gerechtfertigt.

Kodierungsentscheidungen sollten im Großen und Ganzen in erster Linie auf Leistung und Wartbarkeit basieren. Wenn die Leistung nicht auf dem Spiel steht (und ich kann mir nicht vorstellen, wie sie jemals in Ihren Beispielen aussehen könnte), sollte Ihre nächste Überlegung lauten: Wie einfach ist dies für mich (oder einen zukünftigen Redakteur) zu warten? Ist es intuitiv und verständlich? Ist es isoliert? Ihr Beispiel für verkettete Funktionsaufrufe scheint in dieser Hinsicht tatsächlich potenziell spröde zu sein: Wenn Sie sich für eine Änderung von bIsUpin entscheiden bIsDown, wie viele andere Stellen im Code müssen ebenfalls geändert werden? Auch steigt Ihre Parameterliste im Ballon auf? Wenn Ihre Funktion 17 Parameter hat, ist die Lesbarkeit ein Problem, und Sie müssen überdenken, ob Sie die Vorteile der objektorientierten Architektur schätzen.

kmote
quelle
4
Ich schätze die Einschränkung im ersten Absatz. Ich habe absichtlich provoziert, indem ich "falsch" gesagt habe, und ich habe mit Sicherheit eingeräumt, dass es sich um "Best Practices" und Designprinzipien handelt und dass solche Dinge oft situativ sind und man mehrere Faktoren abwägen muss
Ray
13
Ihre Antwort erinnert mich an ein Zitat, an das ich mich nicht erinnern kann, aus welcher Quelle - "Wenn Ihre Funktion 17 Parameter enthält, fehlt wahrscheinlich einer".
Joris Timmermans
Ich stimme dem sehr zu und
wende mich
15

Ich denke, Robert C. Martins Clean-Code-Artikel besagt, dass Sie boolesche Argumente, wenn möglich, entfernen sollten, da sie zeigen, dass eine Methode mehr als eine Sache tut. Eine Methode sollte eine Sache tun und nur eine Sache, die ich denke, ist eines seiner Mottos.

dreza
quelle
@dreza Sie beziehen sich auf Curlys Law .
MattDavey
Natürlich sollten Sie aus Erfahrung wissen, wann Sie solche Argumente ignorieren sollten.
gnasher729
8

Ich denke, das Wichtigste hier ist, praktisch zu sein.

Wenn der Boolesche Wert das gesamte Verhalten bestimmt, erstellen Sie einfach eine zweite Methode.

Wenn der Boolesche Wert nur ein wenig Verhalten in der Mitte festlegt, möchten Sie ihn möglicherweise in einem Zustand belassen, um die Codeduplizierung zu verringern. Möglicherweise können Sie die Methode sogar in drei aufteilen: Zwei aufrufende Methoden für eine der beiden booleschen Optionen und eine, die den Großteil der Arbeit erledigt.

Zum Beispiel:

private void FooInternal(bool flag)
{
  //do work
}

public void Foo1()
{
  FooInternal(true);
}

public void Foo2()
{
  FooInternal(false);
}

Natürlich haben Sie in der Praxis immer einen Punkt zwischen diesen Extremen. Normalerweise gehe ich einfach mit dem um, was sich richtig anfühlt, aber ich bevorzuge es, auf der Seite weniger Code-Duplizierung zu irren.

Jorn
quelle
Ich verwende nur boolesche Argumente, um das Verhalten in privaten Methoden zu steuern (wie hier gezeigt). Aber das Problem: Wenn irgendein Dufus beschließt, die Sichtbarkeit FooInternalin der Zukunft zu erhöhen , was dann?
ADTC
Eigentlich würde ich einen anderen Weg gehen. Der Code in FooInternal sollte in 4 Teile aufgeteilt werden: 2 Funktionen zur Behandlung der Booleschen Wahr / Falsch-Fälle, eine für die Arbeit, die zuvor ausgeführt wurde, und eine für die Arbeit, die danach ausgeführt wurde. Dann Sie Foo1wird: { doWork(); HandleTrueCase(); doMoreWork() }. Im Idealfall werden die Funktionen doWorkund doMoreWorkjeweils in (einen oder mehrere) bedeutungsvolle Teile diskreter Aktionen (dh als separate Funktionen) unterteilt, und nicht nur in zwei Funktionen, um sie aufzuteilen.
Jpaugh
7

Mir gefällt der Ansatz, das Verhalten mithilfe von Builder-Methoden anzupassen, die unveränderliche Instanzen zurückgeben. So benutzt GuavaSplitter es:

private static final Splitter MY_SPLITTER = Splitter.on(',')
       .trimResults()
       .omitEmptyStrings();

MY_SPLITTER.split("one,,,,  two,three");

Die Vorteile davon sind:

  • Hervorragende Lesbarkeit
  • Klare Trennung von Konfigurations- und Aktionsmethoden
  • Fördert den Zusammenhalt, indem Sie gezwungen werden, sich Gedanken darüber zu machen, was das Objekt ist, was es tun sollte und was nicht. In diesem Fall ist es ein Splitter. Sie würden niemals someVaguelyStringRelatedOperation(List<Entity> myEntities)eine Klasse mit dem Namen Splittereinfügen, aber Sie würden darüber nachdenken, sie als statische Methode in eine StringUtilsKlasse einzufügen.
  • Die Instanzen sind daher vorkonfiguriert und können problemlos in Abhängigkeit injiziert werden. Der Client muss sich keine Gedanken darüber machen, ob eine Methode übergeben trueoder übergeben falsewerden soll, um das richtige Verhalten zu erzielen.
oksayt
quelle
1
Ich bin ein Teil Ihrer Lösung als großer Guavenliebhaber und Evangelist ... aber ich kann Ihnen keine +1 geben, weil Sie den Teil überspringen, den ich wirklich suche, was falsch ist (oder stinkt) über den anderen weg. Ich denke, dass dies in einigen Ihrer Erklärungen tatsächlich implizit ist. Wenn Sie dies also explizit ausdrücken könnten, würde dies die Frage möglicherweise besser beantworten.
Ray
Ich mag die Idee, Konfigurations- und Acton-Methoden zu trennen!
Sher10ck
Die Links zur Guava-Bibliothek sind unterbrochen
Josh Noe
4

Auf jeden Fall ein Codegeruch . Wenn es nicht gegen das Prinzip der Einzelverantwortung verstößt , verstößt es wahrscheinlich gegen Tell, Don't Ask . Erwägen:

Wenn sich herausstellt, dass eines dieser beiden Prinzipien nicht verletzt wird, sollten Sie trotzdem eine Aufzählung verwenden. Boolesche Flags sind das boolesche Äquivalent zu magischen Zahlen . foo(false)macht so viel Sinn wie bar(42). Aufzählungen können für Strategiemuster nützlich sein und Sie können flexibel eine weitere Strategie hinzufügen. (Denken Sie daran, sie entsprechend zu benennen .)

Ihr spezielles Beispiel stört mich besonders. Warum wird diese Flagge so oft durchlaufen? Das hört sich so an, als müssten Sie Ihren Parameter in Unterklassen aufteilen .

Eva
quelle
4

TL; DR: Verwenden Sie keine booleschen Argumente.

Sehen Sie unten, warum sie schlecht sind und wie Sie sie ersetzen können (fett gedruckt).


Boolesche Argumente sind sehr schwer zu lesen und daher schwer zu pflegen. Das Hauptproblem ist, dass der Zweck im Allgemeinen klar ist, wenn Sie die Methodensignatur lesen, in der das Argument benannt ist. Die Benennung eines Parameters ist jedoch in den meisten Sprachen nicht erforderlich. Sie haben also Anti-Patterns, bei RSACryptoServiceProvider#encrypt(Byte[], Boolean)denen der boolesche Parameter festlegt, welche Art von Verschlüsselung in der Funktion verwendet werden soll.

So erhalten Sie einen Anruf wie:

rsaProvider.encrypt(data, true);

wo der Leser die Signatur der Methode nachschlagen muss, um festzustellen, was zum Teufel trueeigentlich bedeuten könnte. Eine Ganzzahl zu übergeben ist natürlich genauso schlecht:

rsaProvider.encrypt(data, 1);

würde dir genauso viel sagen - oder vielmehr: genauso wenig. Selbst wenn Sie Konstanten definieren, die für die Ganzzahl verwendet werden sollen, können Benutzer der Funktion diese einfach ignorieren und weiterhin Literalwerte verwenden.

Der beste Weg, dies zu lösen, ist die Verwendung einer Aufzählung . Wenn Sie eine Enumeration RSAPaddingmit zwei Werten übergeben müssen: OAEPoder PKCS1_V1_5dann können Sie den Code sofort lesen:

rsaProvider.encrypt(data, RSAPadding.OAEP);

Boolesche Werte können nur zwei Werte haben. Wenn Sie also eine dritte Option haben, müssen Sie Ihre Signatur überarbeiten. Im Allgemeinen kann dies nicht einfach durchgeführt werden, wenn Abwärtskompatibilität ein Problem ist, sodass Sie eine öffentliche Klasse mit einer anderen öffentlichen Methode erweitern müssen. Dies ist, was Microsoft schließlich tat, als sie einführten, RSACryptoServiceProvider#encrypt(Byte[], RSAEncryptionPadding)wo sie eine Aufzählung (oder zumindest eine Klasse, die eine Aufzählung nachahmt) anstelle eines Booleschen verwendeten.

Es kann sogar einfacher sein, ein vollständiges Objekt oder eine vollständige Schnittstelle als Parameter zu verwenden, falls der Parameter selbst parametrisiert werden muss. Im obigen Beispiel könnte die OAEP-Auffüllung selbst mit dem Hash-Wert parametrisiert werden, der intern verwendet wird. Beachten Sie, dass es jetzt 6 SHA-2-Hash-Algorithmen und 4 SHA-3-Hash-Algorithmen gibt, sodass die Anzahl der Aufzählungswerte explodieren kann, wenn Sie nur eine einzelne Aufzählung anstelle von Parametern verwenden (dies ist möglicherweise das nächste, was Microsoft herausfinden wird ).


Boolesche Parameter können auch darauf hinweisen, dass die Methode oder Klasse nicht gut entworfen wurde. Wie im obigen Beispiel: Jede andere kryptografische Bibliothek als .NET verwendet überhaupt kein Füllflag in der Methodensignatur.


Fast alle Software-Gurus, die ich mag, warnen vor booleschen Argumenten. Zum Beispiel warnt Joshua Bloch in dem hochgeschätzten Buch "Effective Java" davor. Im Allgemeinen sollten sie einfach nicht verwendet werden. Sie könnten argumentieren, dass sie verwendet werden könnten, wenn es einen Parameter gibt, der leicht zu verstehen ist. Aber selbst dann: Bit.set(boolean)wird wahrscheinlich besser mit zwei Methoden implementiert : Bit.set()und Bit.unset().


Wenn Sie den Code nicht direkt umgestalten können, können Sie Konstanten definieren , um sie zumindest lesbarer zu machen:

const boolean ENCRYPT = true;
const boolean DECRYPT = false;

...

cipher.init(key, ENCRYPT);

ist viel lesbarer als:

cipher.init(key, true);

auch wenn du lieber hättest:

cipher.initForEncryption(key);
cipher.initForDecryption(key);

stattdessen.

Maarten Bodewes
quelle
3

Ich bin überrascht, dass niemand Named-Parameter erwähnt hat .

Das Problem, das ich bei Booleschen Flags sehe, ist, dass sie die Lesbarkeit beeinträchtigen. Zum Beispiel, was macht truein

myObject.UpdateTimestamps(true);

machen? Ich habe keine Ahnung. Aber was ist mit:

myObject.UpdateTimestamps(saveChanges: true);

Jetzt ist klar, was der Parameter, den wir übergeben, bewirken soll: Wir weisen die Funktion an, ihre Änderungen zu speichern. In diesem Fall, wenn die Klasse nicht öffentlich ist, denke ich, dass der boolesche Parameter in Ordnung ist.


Natürlich können Sie die Benutzer Ihrer Klasse nicht zwingen, Named-Parameter zu verwenden. Aus diesem Grund enumsind in der Regel ein oder zwei separate Methoden vorzuziehen, je nachdem, wie häufig Ihr Standardfall ist. Genau das macht .Net:

//Enum used
double a = Math.Round(b, MidpointRounding.AwayFromZero);

//Two separate methods used
IEnumerable<Stuff> ascendingList = c.OrderBy(o => o.Key);
IEnumerable<Stuff> descendingList = c.OrderByDescending(o => o.Key); 
BlueRaja - Danny Pflughoeft
quelle
1
Die Frage ist nicht, was einer verhaltensbestimmenden Flagge vorzuziehen ist, es ist, ob eine solche Flagge ein Geruch ist und wenn ja, warum
Ray
2
@ Ray: Ich sehe keinen Unterschied zwischen diesen beiden Fragen. In einer Sprache, in der Sie die Verwendung benannter Parameter erzwingen können oder in der Sie sicher sein können, dass immer benannte Parameter verwendet werden (z. B. private Methoden) , sind boolesche Parameter in Ordnung. Wenn benannte Parameter nicht von der Sprache (C #) erzwungen werden können und die Klasse Teil einer öffentlichen API ist oder wenn die Sprache benannte Parameter (C ++) nicht unterstützt, sodass Code wie geschrieben myFunction(true)möglicherweise ein Code ist, Geruch.
BlueRaja - Danny Pflughoeft
Der genannte Parameter-Ansatz ist noch falscher. Ohne einen Namen wird man gezwungen, das API-Dokument zu lesen. Mit einem Namen glaubst du, dass du ihn nicht brauchst: aber der Parameter könnte falsch benannt sein. Beispielsweise könnte es verwendet werden, um (alle) Änderungen zu speichern, aber später hat sich die Implementierung ein wenig geändert, um nur große Änderungen (für einen bestimmten Wert von big) zu speichern .
Ingo
@Ingo Ich stimme nicht zu. Das ist ein generisches Programmierproblem. Sie können leicht einen anderen SaveBigNamen definieren . Jeder Code kann vermasselt werden, diese Art der Vermasselung bezieht sich nicht auf benannte Parameter.
Maarten Bodewes
1
@Ingo: Wenn du von Idioten umgeben bist, dann gehst du woanders hin und suchst dir einen Job. Dafür haben Sie Code-Reviews.
gnasher729
1

Ich kann nicht genau sagen, was daran falsch ist.

Wenn es wie ein Codegeruch aussieht, sich wie ein Codegeruch anfühlt und - nun - wie ein Codegeruch riecht, ist es wahrscheinlich ein Codegeruch.

Was Sie tun möchten, ist:

1) Vermeiden Sie Methoden mit Nebenwirkungen.

2) Behandeln Sie notwendige Zustände mit einer zentralen, formalen Zustandsmaschine ( so ).

BaBu
quelle
1

Ich bin mit allen Bedenken einverstanden, Boolesche Parameter zu verwenden, um nicht die Leistung zu bestimmen, um; Verbesserung, Lesbarkeit, Zuverlässigkeit, Verringerung der Komplexität, Verringerung der Risiken durch schlechte Verkapselung und Kohäsion sowie Senkung der Gesamtbetriebskosten durch Wartbarkeit.

Ich begann Mitte der 70er Jahre mit der Entwicklung von Hardware, die wir heute SCADA (Supervisor Control and Data Acquisition) nennen. Dabei handelte es sich um fein abgestimmte Hardware mit Maschinencode im EPROM, auf der Makro-Fernbedienungen ausgeführt und Hochgeschwindigkeitsdaten erfasst wurden.

Die Logik heißt Mealey & Moore- Maschinen, die wir jetzt Finite-State-Maschinen nennen . Diese müssen ebenfalls nach den gleichen Regeln wie oben ausgeführt werden, es sei denn, es handelt sich um eine Echtzeitmaschine mit einer begrenzten Ausführungszeit, und dann müssen Verknüpfungen ausgeführt werden, um diesen Zweck zu erfüllen.

Die Daten sind synchron, aber die Befehle sind asynchron, und die Befehlslogik folgt der speicherlosen Booleschen Logik, jedoch mit sequentiellen Befehlen, die auf dem Speicher des vorherigen, gegenwärtigen und gewünschten nächsten Zustands basieren. Damit dies in der effizientesten Maschinensprache (nur 64 KB) funktioniert, wurde große Sorgfalt darauf verwendet, jeden Prozess auf heuristische Weise für IBM HIPO zu definieren. Das bedeutete manchmal, boolesche Variablen zu übergeben und indizierte Verzweigungen auszuführen.

Aber jetzt mit viel Speicher und einfacher OOK, ist Encapsulation heute ein wesentlicher Bestandteil, aber eine Strafe, wenn Code in Bytes für Echtzeit- und SCADA-Maschinencode gezählt wurde.

Tony Stewart Sunnyskyguy EE75
quelle
0

Es ist nicht unbedingt falsch, aber in Ihrem konkreten Beispiel für die Aktion, die von einem Attribut des "Benutzers" abhängt, würde ich eher einen Verweis auf den Benutzer als ein Flag übergeben.

Dies verdeutlicht und hilft in mehrfacher Hinsicht.

Jeder, der die aufrufende Anweisung liest, wird feststellen, dass sich das Ergebnis je nach Benutzer ändert.

In der Funktion, die letztendlich aufgerufen wird, können Sie einfach komplexere Geschäftsregeln implementieren, da Sie auf alle Benutzerattribute zugreifen können.

Wenn eine Funktion / Methode in der "Kette" in Abhängigkeit von einem Benutzerattribut etwas anderes ausführt, ist es sehr wahrscheinlich, dass eine ähnliche Abhängigkeit von Benutzerattributen für einige der anderen Methoden in der "Kette" eingeführt wird.

James Anderson
quelle
0

Meistens würde ich diese schlechte Kodierung in Betracht ziehen. Ich kann mir jedoch zwei Fälle vorstellen, in denen dies eine gute Praxis sein könnte. Da es bereits viele Antworten gibt, die sagen, warum es schlecht ist, biete ich zwei Mal an, wenn es gut sein könnte:

Der erste ist, wenn jeder Aufruf in der Sequenz für sich genommen Sinn macht. Es wäre sinnvoll , wenn der anrufende Code könnte von true in false oder falsch wahr, geändert werden oder wenn die aufgerufene Methode könne geändert werden als Weitergabe direkt vielmehr den Booleschen Parameter zu verwenden. Die Wahrscheinlichkeit von zehn solchen Aufrufen hintereinander ist gering, aber es könnte passieren, und wenn dies der Fall wäre, wäre es eine gute Programmierpraxis.

Der zweite Fall ist etwas schwierig, da es sich um einen Booleschen handelt. Wenn das Programm jedoch mehrere Threads oder Ereignisse enthält, ist die Übergabe von Parametern der einfachste Weg, um thread- / ereignisspezifische Daten zu verfolgen. Beispielsweise kann ein Programm Eingaben von zwei oder mehr Sockets erhalten. Code, der für einen Socket ausgeführt wird, muss möglicherweise Warnmeldungen ausgeben, Code, der für einen anderen Socket ausgeführt wird, möglicherweise nicht. Es ist dann (irgendwie) sinnvoll, wenn ein Boolescher Satz auf einer sehr hohen Ebene durch viele Methodenaufrufe an die Stellen geleitet wird, an denen möglicherweise Warnmeldungen generiert werden. Die Daten können nicht (außer unter großen Schwierigkeiten) in irgendeiner Art von Global gespeichert werden, da mehrere Threads oder verschachtelte Ereignisse jeweils ihren eigenen Wert benötigen würden.

In letzterem Fall würde ich wahrscheinlich eine Klasse / Struktur erstellen, deren einziger Inhalt ein Boolescher Wert ist, und diesen stattdessen weitergeben. Ich bin mir ziemlich sicher, dass ich in Kürze andere Felder benötige - zum Beispiel, wohin die Warnmeldungen gesendet werden sollen.

RalphChapin
quelle
Eine Aufzählung WARN, SILENT wäre sinnvoller, auch wenn Sie eine Klasse / Struktur verwenden, wie Sie sie geschrieben haben (dh einen Kontext ). Oder konfigurieren Sie die Protokollierung einfach extern - Sie müssen nichts weiter übergeben.
Maarten Bodewes
-1

Der Kontext ist wichtig. Solche Methoden sind in iOS ziemlich verbreitet. Als nur ein häufig verwendetes Beispiel stellt UINavigationController die Methode bereit -pushViewController:animated:, und der animatedParameter ist ein BOOL. Die Methode führt in beiden Fällen im Wesentlichen dieselbe Funktion aus, animiert jedoch den Übergang von einem Ansichtscontroller zu einem anderen, wenn Sie JA übergeben, und nicht, wenn Sie NEIN übergeben. Dies scheint völlig vernünftig; Es wäre albern, zwei Methoden anstelle dieser bereitzustellen, damit Sie bestimmen können, ob Sie Animation verwenden möchten oder nicht.

In Objective-C ist dies möglicherweise einfacher zu rechtfertigen, da die Syntax zur Benennung von Methoden mehr Kontext für jeden Parameter bietet als in Sprachen wie C und Java. Trotzdem würde ich denken, dass eine Methode, die einen einzelnen Parameter verwendet, leicht einen Booleschen Wert annehmen kann und dennoch Sinn ergibt:

file.saveWithEncryption(false);    // is there any doubt about the meaning of 'false' here?
Caleb
quelle
21
Eigentlich habe ich keine Ahnung, was falsedas im file.saveWithEncryptionBeispiel bedeutet. Bedeutet das, dass es ohne Verschlüsselung gespeichert wird? Wenn ja, warum sollte die Methode "mit Verschlüsselung" im Namen stimmen? Ich könnte eine Methode wie diese verstehen save(boolean withEncryption), aber wenn ich sie sehe file.save(false), ist es auf den ersten Blick nicht offensichtlich, dass der Parameter angibt, dass es sich um eine verschlüsselte oder unverschlüsselte Methode handelt . Ich denke in der Tat, das macht James Youngman zum ersten Mal darauf an, eine Aufzählung zu verwenden.
Ray
6
Eine alternative Hypothese ist, dass falsekeine existierende Datei mit dem gleichen Namen überschrieben wird. Ich kenne ein erfundenes Beispiel, aber um sicherzugehen, müssen Sie die Dokumentation (oder den Code) für die Funktion überprüfen.
James Youngman
5
Eine Methode mit dem Namen saveWithEncryption, die manchmal nicht mit Verschlüsselung gespeichert wird, ist ein Fehler. Es sollte möglich sein file.encrypt().save(), oder wie Javas new EncryptingOutputStream(new FileOutputStream(...)).write(file).
Christoffer Hammarström
2
Tatsächlich funktioniert Code, der etwas anderes tut als das, was er sagt, nicht und ist daher ein Fehler. Es fliegt nicht, um eine Methode mit dem Namen saveWithEncryption(boolean)zu erstellen, die manchmal ohne Verschlüsselung gespeichert wird, genauso wie es nicht fliegt, um eine Methode zu erstellen, saveWithoutEncryption(boolean)die manchmal mit Verschlüsselung gespeichert wird.
Christoffer Hammarström
2
Die Methode ist schlecht, weil offensichtlich "Zweifel an der Bedeutung von" falsch "bestehen". Ich würde sowieso niemals eine solche Methode schreiben. Speichern und Verschlüsseln sind separate Aktionen, und eine Methode sollte eine Sache tun und es gut machen. In meinen früheren Kommentaren finden Sie bessere Beispiele.
Christoffer Hammarström