Ist diese Verwendung von Bedingungen ein Anti-Muster?

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.

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.

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.

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);

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

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.

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.

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.

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.

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.

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.

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.

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.