Ist diese Verwendung von Bedingungen ein Anti-Muster?

14

Ich habe dies in unserem Legacy-System bei der Arbeit viel gesehen - Funktionen, die ungefähr so ​​aussehen:

bool todo = false;
if(cond1)
{
  ... // lots of code here
  if(cond2)
    todo = true;
  ... // some other code here
}

if(todo)
{
  ...
}

Mit anderen Worten, die Funktion besteht aus zwei Teilen. Der erste Teil führt eine Art von Verarbeitung durch (die möglicherweise Schleifen, Nebenwirkungen usw. enthält) und setzt dabei möglicherweise das "todo" -Flag. Der zweite Teil wird nur ausgeführt, wenn das Flag "todo" gesetzt ist.

Es scheint ein ziemlich hässlicher Weg zu sein, Dinge zu tun, und ich denke, die meisten Fälle, für die ich mir Zeit genommen habe, um sie zu verstehen, könnten überarbeitet werden, um die Verwendung der Flagge zu vermeiden. Aber ist das wirklich ein Gegenmuster, eine schlechte Idee oder vollkommen akzeptabel?

Die erste offensichtliche Umgestaltung wäre, es in zwei Methoden zu schneiden. In meiner Frage geht es jedoch eher darum, ob es jemals erforderlich ist (in einer modernen OO-Sprache), eine lokale Flag-Variable zu erstellen, die möglicherweise an mehreren Stellen gesetzt und später verwendet wird, um zu entscheiden, ob der nächste Codeblock ausgeführt werden soll.

Kricket
quelle
2
Wie umgestalten Sie es?
Tamás Szelei
13
Unter der Annahme, dass das ToDo an mehreren Stellen stattfindet, kann ich mir kaum ein Refactoring vorstellen, das den geringsten Sinn ergibt. Wenn es kein Refactoring gibt, gibt es kein Antimuster. Außer der Benennung der todo-Variablen; sollte aussagekräftiger benannt werden, wie "doSecurityCheck".
user281377
3
@ammoQ: +1; Wenn die Dinge kompliziert sind, sind sie so. Eine Flag-Variable kann unter bestimmten Umständen viel sinnvoller sein, da sie klarer macht, dass eine Entscheidung getroffen wurde, und Sie können danach suchen, um herauszufinden, wo diese Entscheidung getroffen wurde.
Donal Fellows
1
@Donal Fellows: Wenn die Suche nach dem Grund notwendig ist, würde ich die Variable zu einer Liste machen; Solange es leer ist, ist es "falsch". Wo immer das Flag gesetzt ist, wird ein Ursachencode zur Liste hinzugefügt. Sie könnten also mit einer solchen Liste enden ["blacklisted-domain","suspicious-characters","too-long"], die zeigt, dass mehrere Gründe zutrafen.
user281377
2
Ich denke nicht, dass es ein Anti-Muster ist, aber es ist definitiv ein Geruch
Binary Worrier

Antworten:

23

Ich weiß nichts über Anti-Muster, aber ich würde drei Methoden daraus extrahieren.

Der erste würde etwas Arbeit verrichten und einen booleschen Wert zurückgeben.

Die zweite würde die Arbeit ausführen, die von "einem anderen Code" ausgeführt wird.

Der dritte würde die Hilfsarbeit ausführen, wenn der zurückgegebene Boolesche Wert wahr wäre.

Die extrahierten Methoden wären wahrscheinlich privat, wenn es wichtig wäre, dass die zweite Methode nur (und immer) aufgerufen wird, wenn die erste Methode true zurückgibt.

Wenn ich die Methoden gut bezeichne, hoffe ich, dass sie den Code klarer machen.

Etwas wie das:

public void originalMethod() {
    bool furtherProcessingRequired = lotsOfCode();
    someOtherCode();
    if (furtherProcessingRequired) {
        doFurtherProcessing();
    }
    return;
}

private boolean lotsOfCode() {
    if (cond1) {
        ... // lots of code here
        if(cond2) {
            return true;
        }
    }
    return false;
}

private void someOtherCode() {
    ... // some other code here
}

private void doFurtherProcessing() {
    // Do whatever is needed
}

Offensichtlich ist umstritten, ob eine vorzeitige Rückgabe akzeptabel ist, aber dies ist ein Implementierungsdetail (wie auch der Standard für die Code-Formatierung).

Punkt ist, dass die Absicht des Codes klarer wird, was gut ist ...

Einer der Kommentare zu dieser Frage legt nahe, dass dieses Muster einen Geruch darstellt , und dem würde ich zustimmen. Es lohnt sich zu prüfen, ob Sie die Absicht klarer machen können.

Bill Michell
quelle
Das Aufteilen in zwei Funktionen würde immer noch eine todoVariable erfordern und wäre wahrscheinlich schwerer zu verstehen.
Pubby
Ja, das würde ich auch tun, aber meine Frage bezog sich mehr auf die Verwendung der "todo" -Flagge.
Kricket
2
Wenn Sie mit enden if (check_if_needed ()) do_whatever ();, gibt es dort keine offensichtliche Flagge. Ich denke, dies kann den Code zu sehr aufbrechen und möglicherweise die Lesbarkeit beeinträchtigen, wenn der Code einigermaßen einfach ist. Schließlich können sich die Details Ihrer Aktionen auf die Testdurchführung do_whateverauswirken check_if_needed, sodass es nützlich ist, den gesamten Code auf demselben Bildschirm zusammenzuhalten. Dies garantiert auch nicht, dass check_if_neededdie Verwendung eines Flags vermieden werden kann - und wenn dies der Fall ist, werden wahrscheinlich mehrere returnAnweisungen verwendet, was möglicherweise strenge Befürworter eines einzelnen Ausgangs verärgert.
Steve314
3
@ Pubby8 sagte er "extrahiere 2 Methoden daraus" , was zu 3 Methoden führte. 2 Methoden, die die eigentliche Verarbeitung durchführen, und die ursprüngliche Methode, die den Workflow koordiniert. Dies wäre ein viel saubereres Design.
MattDavey
Dies lässt den ... // some other code hereFall der vorzeitigen Rückgabe
Caleth
6

Ich denke, die Hässlichkeit ist auf die Tatsache zurückzuführen, dass eine einzige Methode viel Code enthält und / oder Variablen schlecht benannt sind (beides sind eigenständige Code-Gerüche - Antimuster sind abstraktere und komplexere Dinge, IMO).

Wenn Sie also den größten Teil des Codes in Methoden auf niedrigerer Ebene extrahieren, wie @Bill vorschlägt, wird der Rest sauber (zumindest für mich). Z.B

bool registrationNeeded = installSoftware(...);
if (registrationNeeded) {
  registerUser(...)
}

Oder Sie können die lokale Flagge sogar vollständig entfernen, indem Sie den Flag-Check in der zweiten Methode ausblenden und ein Formular wie das folgende verwenden

calculateTaxRefund(isTaxRefundable(...), ...)

Insgesamt sehe ich eine lokale Flag-Variable nicht unbedingt als schlecht an. Welche der obigen Optionen besser lesbar ist (= mir vorzuziehen), hängt von der Anzahl der Methodenparameter, den gewählten Namen und der Form ab, die der inneren Logik des Codes besser entspricht.

Péter Török
quelle
4

todo ist ein wirklich schlechter Name für die Variable, aber ich denke, das könnte alles sein, was falsch ist. Ohne den Kontext ist es schwer, ganz sicher zu sein.

Nehmen wir an, der zweite Teil der Funktion sortiert eine Liste, die aus dem ersten Teil besteht. Dies sollte viel besser lesbar sein:

bool requiresSorting = false;
if(cond1)
{
    ... // lots of code here
    if(cond2)
        requiresSorting = true;
    ... // some other code here
}

if(requiresSorting)
{
    ...
}

Bills Vorschlag ist jedoch auch richtig. Dies ist noch lesbarer:

bool requiresSorting = BuildList(list);
if (requiresSorting)
    SortList(list);
pdr
quelle
Warum nicht einfach einen Schritt weiter gehen: if (BuildList (Liste)) SortList (Liste);
Phil N DeBlanc
2

Das Zustandsmaschinenmuster sieht für mich gut aus. Die Anti-Muster sind "todo" (falscher Name) und "viel Code".

ptyx
quelle
Ich bin mir jedoch sicher, dass dies nur zur Veranschaulichung dient.
Loren Pechtel
1
Einverstanden. Ich habe versucht zu vermitteln, dass gute Muster, die in schlechtem Code ertrunken sind, nicht für die Qualität des Codes verantwortlich gemacht werden sollten.
Ptyx
1

Es kommt wirklich darauf an. Wenn der von geschützte Code todo(ich hoffe, Sie verwenden diesen Namen nicht wirklich, da er völlig unmnemonisch ist!) Konzeptionell bereinigender Code ist, dann haben Sie ein Anti-Pattern und sollten so etwas wie C ++ 's RAII oder C #' s verwenden usingkonstruieren, um Dinge stattdessen zu handhaben.

Wenn es sich jedoch konzeptionell nicht um eine Bereinigungsphase handelt, sondern nur um eine zusätzliche Verarbeitung, die manchmal erforderlich ist und bei der die Entscheidung früher getroffen werden muss, ist das, was geschrieben wird, in Ordnung. Überlegen Sie sich, ob einzelne Codeblöcke natürlich besser in ihre eigenen Funktionen umgestaltet werden könnten und ob Sie die Bedeutung der Flag-Variablen in ihrem Namen erfasst haben, aber dieses grundlegende Codemuster ist in Ordnung. Insbesondere könnte der Versuch, zu viel in andere Funktionen zu stecken, die Klarheit des Geschehens beeinträchtigen, und das wäre definitiv ein Anti-Pattern.

Donal Fellows
quelle
Es ist eindeutig keine Bereinigung - es läuft nicht immer. Ich habe bereits Fälle wie diesen getroffen - während Sie etwas verarbeiten, kann dies dazu führen, dass ein vorberechnetes Ergebnis ungültig wird. Wenn die Berechnung teuer ist, möchten Sie sie nur bei Bedarf ausführen.
Loren Pechtel
1

Viele der Antworten hier hätten Probleme, eine Komplexitätsprüfung zu bestehen, einige sahen> 10 aus.

Ich denke, dies ist der "Anti-Muster" -Teil dessen, was Sie betrachten. Finden Sie ein Tool, das die zyklomatische Komplexität Ihres Codes misst - es gibt Plugins für Eclipse. Es ist im Wesentlichen ein Maß dafür, wie schwierig es ist, Ihren Code zu testen, und umfasst die Anzahl und die Ebenen von Codezweigen.

Insgesamt lässt das Layout Ihres Codes mich an "Aufgaben" denken, wenn dies an vielen Stellen geschieht, ist es möglicherweise eine aufgabenorientierte Architektur, von der jede Aufgabe ihre eigene ist Objekt und in der Mitte der Aufgabe haben Sie die Möglichkeit, die nächste Aufgabe in die Warteschlange zu stellen, indem Sie ein anderes Aufgabenobjekt instanziieren und es in die Warteschlange werfen. Diese lassen sich erstaunlich einfach einrichten und reduzieren die Komplexität bestimmter Codetypen erheblich - aber wie ich bereits sagte, ist dies ein absoluter Stich in die Dunkelheit.

Bill K
quelle
1

Anhand des obigen Beispiels von pdr, da es ein schönes Beispiel ist, gehe ich noch einen Schritt weiter.

Er hatte:

bool requiresSorting = BuildList(list);
if (requiresSorting)
    SortList(list);

Also wurde mir klar, dass Folgendes funktionieren würde:

if(BuildList(list)) 
    SortList(list)

Ist aber nicht so klar.

Also zur ursprünglichen Frage, warum nicht:

BuildList(list)
SortList(list)

Und lassen Sie SortList entscheiden, ob eine Sortierung erforderlich ist?

Sie sehen, Ihre BuildList-Methode scheint etwas über das Sortieren zu wissen, da sie einen Bool zurückgibt, der dies anzeigt. Dies ist jedoch für eine Methode, die zum Erstellen einer Liste entwickelt wurde, nicht sinnvoll.

Ian
quelle
Und der nächste Schritt ist natürlich die Frage, warum dies ein zweistufiger Prozess ist. Überall dort, wo ich Code wie diesen sehe, verändere ich die Arbeit an einer Methode namens BuildAndSortList (Liste)
Ian,
Dies ist keine Antwort. Sie haben das Verhalten des Codes geändert.
D Drmmr
Nicht wirklich. Wieder kann ich nicht glauben, dass ich auf etwas antworte, das ich vor 7 Jahren gepostet habe, aber was zum Teufel :) Was ich argumentiert habe, ist, dass SortList die Bedingung enthalten würde. Wenn Sie einen Komponententest durchgeführt hätten, bei dem festgestellt wurde, dass die Liste nur sortiert wurde, wenn die Bedingung x erfüllt war, würde sie weiterhin bestehen. Indem Sie die Bedingung in SortList verschieben, müssen Sie nicht immer schreiben (if (something) then SortList (...))
Ian
0

Ja, dies scheint ein Problem zu sein, da Sie alle Stellen verfolgen müssen, an denen Sie die Flagge EIN / AUS markieren. Es ist besser, die Logik nur als verschachtelte if-Bedingung einzufügen, anstatt die Logik zu entfernen.

Auch Rich-Domain-Modelle, in diesem Fall macht nur ein Liner große Dinge im Objekt.

java_mouse
quelle
0

Wenn das Flag nur einmal gesetzt ist, verschieben Sie den Code
...
bis direkt nach dem
Code ... //,
und beseitigen Sie das Flag.

Ansonsten teilen Sie den
... // viel Code hier
... // etwas anderen Code hier
...
Code nach Möglichkeit in separate Funktionen auf. Es ist also klar, dass diese Funktion eine Verantwortung hat, nämlich die Verzweigungslogik.

Wo immer möglich, trennen Sie den Code in
... // viel Code hier
in zwei oder mehr Funktionen auf, von denen einige Arbeit leisten (was ein Befehl ist) und andere entweder den Wert todo zurückgeben (was eine Abfrage ist) oder ihn ausführen sehr explizit ändern sie es (eine Abfrage mit Nebenwirkungen)

Der Code selbst ist nicht das Anti-Pattern, nach dem Sie suchen.

Andrew Pate
quelle
Was fügt dieser Beitrag hinzu, dass vorhandene Antworten fehlen?
Esoterik
@esoterik Manchmal wird die Möglichkeit, ein wenig CQRS hinzuzufügen, häufig übersehen, wenn es um Flags geht. Die Logik, ein Flag zu ändern, stellt eine Abfrage dar, während das Ausführen von Arbeiten einen Befehl darstellt. Manchmal kann die Trennung der beiden den Code klarer machen. Auch im obigen Code sollte darauf hingewiesen werden, dass es vereinfacht werden kann, da das Flag nur in einem Zweig gesetzt ist. Ich glaube, Flaggen sind kein Gegenmuster und wenn ihr Name den Code tatsächlich ausdrucksvoller macht, sind sie eine gute Sache. Ich denke, wo Flags erstellt, gesetzt und verwendet werden, sollte der Code möglichst dicht beieinander liegen.
Andrew Pastete
0

Manchmal muss ich dieses Muster implementieren. Manchmal möchten Sie mehrere Überprüfungen durchführen, bevor Sie mit einer Operation fortfahren. Aus Effizienzgründen werden Berechnungen mit bestimmten Überprüfungen nur durchgeführt, wenn dies unbedingt erforderlich erscheint. So sehen Sie normalerweise Code wie:

// Check individual fields for proper input

if(fieldsValidated) {
  // Perform cross-checks to see if input contains values which exist in the database

  if(valuesExist) {
    try {
      // Attempt insertion
      trx.commit();
    } catch (DatabaseException dbe) {
      trx.rollback();
      throw dbe;
    }
  } else {
    closeConnection(db);
    throwException();
  }
} else {
  closeConnection(db);
  throwException();
}

Dies könnte vereinfacht werden, indem die Validierung vom eigentlichen Vorgang der Ausführung des Vorgangs getrennt wird.

boolean proceed = true;
// Check individual fields for proper input

if(fieldsValidated) {
  // Perform cross-checks to see if input contains values which exist in the database

  if(!valuesExist) {
    proceed = false;
  }
} else {
  proceed = false;
}

// The moment of truth
if(proceed) {
  try {
    // Attempt insertion
    trx.commit();
  } catch (DatabaseException dbe) {
    trx.rollback();
    throw dbe;
  }
} else {
  if(db.isOpen()) {
    closeConnection(db);
  }
  throwException();
}

Offensichtlich hängt es davon ab, was Sie erreichen möchten. Obwohl Sie so geschrieben haben, werden sowohl Ihr "Erfolgs" -Code als auch Ihr "Fehler" -Code einmal geschrieben, was die Logik vereinfacht und das gleiche Leistungsniveau beibehält. Von dort aus wäre es eine gute Idee, ganze Validierungsebenen in innere Methoden einzubauen, die boolesche Anzeigen für Erfolg oder Misserfolg zurückgeben, die die Dinge weiter vereinfachen, obwohl einige Programmierer aus seltsamen Gründen extrem lange Methoden mögen.

Neil
quelle
In dem Beispiel, das Sie angegeben haben, würde ich es vorziehen, eine Funktion shouldIDoIt (fieldsValidated, valuesExist) zu haben, die die Antwort zurückgibt. Dies liegt daran, dass die Ja / Nein-Bestimmung alle auf einmal erfolgt, im Gegensatz zu dem Code, den ich hier bei der Arbeit sehe, wo die Entscheidung, fortzufahren, auf einige verschiedene nicht zusammenhängende Stellen verteilt ist.
Kricket
@ KelseyRider, das war genau der Punkt. Durch die Trennung von Validierung und Ausführung können Sie die Logik in eine Methode einfügen, um die Gesamtlogik des Programms in if (isValidated ()) doOperation ()
Neil,
0

Dies ist kein Muster . Die allgemeinste Interpretation ist, dass Sie eine boolesche Variable setzen und später auf ihren Wert verzweigen. Das ist normale prozedurale Programmierung, nichts weiter.

Ihr spezielles Beispiel kann nun wie folgt umgeschrieben werden:

if(cond1)
{
    ... // lots of code here
    ... // some other code here
    if (cond2)
    {
        ...
    }
}

Das ist vielleicht leichter zu lesen. Oder vielleicht nicht. Dies hängt vom Rest des Codes ab, den Sie ausgelassen haben. Konzentrieren Sie sich darauf, den Code prägnanter zu gestalten.

D Dr.
quelle
-1

Lokale Flags, die für den Kontrollfluss verwendet werden, sollten als eine Form der gotoTarnung erkannt werden . Wenn ein Flag nur innerhalb einer Funktion verwendet wird, kann es beseitigt werden, indem zwei Kopien der Funktion geschrieben werden, eine als "Flag ist wahr" und die andere als "Flag ist falsch" gekennzeichnet wird und jede Operation, die das Flag setzt, ersetzt wird Wenn es klar ist oder wenn es gesetzt ist, wird es mit einem Sprung zwischen den beiden Versionen der Funktion gelöscht.

In vielen Fällen ist Code, der ein Flag verwendet, sauberer als jede andere Alternative, die gotostattdessen verwendet wird. Dies ist jedoch nicht immer der Fall. In einigen Fällen ist gotodas Überspringen eines Codeteils sauberer als das Verwenden von Flags (obwohl einige Leute hier möglicherweise einen bestimmten Raptor-Cartoon einfügen).

Meines Erachtens sollte das primäre Leitprinzip darin bestehen, dass der Programmlogikfluss der Beschreibung der Geschäftslogik so weit wie möglich ähnelt. Wenn die Anforderungen an die Geschäftslogik in Form von Zuständen beschrieben werden, die auf seltsame Weise aufgeteilt und zusammengeführt werden, ist es möglicherweise auch sauberer, wenn die Programmlogik dies tut, als zu versuchen, Flags zu verwenden, um eine solche Logik auszublenden. Auf der anderen Seite wäre es die natürlichste Art, Geschäftsregeln zu beschreiben, zu sagen, dass eine Aktion ausgeführt werden sollte, wenn bestimmte andere Aktionen ausgeführt wurden. Die natürlichste Art, dies auszudrücken, könnte darin bestehen, ein Flag zu verwenden, das beim Ausführen gesetzt wird Letzteres wirkt und ist ansonsten klar.

Superkatze
quelle