meinen Code Umgestalten: Bedingungen basierend auf verschiedenen Variablen

Question

Gegeben:

internal void Configure(ButtonEventArgs args, IBroker broker, FunctionEntry entry) 
     { 
      int phase = broker.TradingPhase; 

      if (args.Button == ItemType.SendAutoButton) 
      {        
       if (phase == 1) 
       { 
        entry.SetParameter("ANDealerPrice", -1); 
        entry.SetParameter("ANAutoUpdate", 4); 
       } 
       else if (phase == 2) 
       { 
        entry.SetParameter("ANDealerPrice", -1); 
        entry.SetParameter("ANAutoUpdate", 2); 
       } 
      } 

      if (phase == 1) 
      { 
       if (broker.IsCashBMK) 
       { 
        entry.SetParameter("Value", 100); 

       } 
       else if (broker.IsCross) 
       { 

        entry.SetParameter("Value", 200); 

       } 
      } 
} 

Ich suche Anregungen den obigen Code Refactoring. Wie von Fowler vorgeschlagen: "Ersetze Bedingung durch Strategie/Polymorphie", kann ich keinen effektiven Code in diesen Zeilen implementieren. Da es mehrere Bedingungen gibt, basieren Sie auf mehreren Variablen.

Bitte schlagen Sie vor, ob es ein Muster geben könnte, das diese fehleranfälligen und hässlichen Bedingungen beseitigen könnte (Code Geruch).

Vielen Dank für Ihr Interesse.

Edit: 1) Meine Absicht ist es, Open-Closed-Prinzip hier zu verwenden, so dass, wenn morgen eine Änderung in der Logik ich diese Bedingungen durch Einführung einer neuen Klasse erweitern kann. 2) Bitte beachten Sie die magischen Zahlen, im realen Szenario habe ich gültige Konstante/Quelle für sie.

Answer 1

Für das, was Sie bisher vorgestellt habe, würde ich geneigt sein, die drei Parameter zu trennen, die jeweils in eine eigene Funktion, also:

void SetAnDealerPrice(ButtonEventArgs args, IBroker broker, 
     FunctionEntry entry) { 
    if (args.Button != ItemType.SendAutoButton) 
     return; 
    int phase = broker.TradingPhase; 
    if (phase == 1 || phase == 2) 
     entry.SetParameter("ANDealerPrice", -1); 
} 

void SetAnAutoUpdate(ButtonEventArgs args, IBroker broker, 
     FunctionEntry entry) { 
    if (args.Button != ItemType.SendAutoButton) 
     return; 
    switch (broker.TradingPhase) { 
    case 1: 
     entry.SetParameter("ANAutoUpdate", 4); 
     break; 
    case 2: 
     entry.SetParameter("ANAutoUpdate", 2); 
     break; 
    } 
} 

void SetValue(IBroker broker, FunctionEntry entry) { 
    if (broker.TradingPhase != 1) 
     return; 
    entry.SetParameter("Value", broker.IsCashBMK ? 100 : 200); 
} 

Dies ist etwas handgefertigte (nicht verleihen selbst bis zur halbautomatischen Aktualisierung, wenn Regeln sich ändern), und marginal weniger effizient (einige Bedingungen werden überprüft, und einige Felder werden mehrfach referenziert, und natürlich sind es mehr Funktionsaufrufe). Ich glaube nicht, dass Effizienz zählt, bis Sie ein Problem haben (und wenn Sie dies tun, größere Änderungen als diese erforderlich sind), und dieser Ansatz lässt Sie genau wissen, welchen Code Sie betrachten müssen, wenn sich die Regeln für einen bestimmten Parameter ändern. Ich glaube nicht, dass Polymorphismus hier zu einer guten Lösung führen wird.

Answer 2

Es sieht für mich so aus, als würde die ganze Funktion die String-Parameter von FunctionEntry setzen.
Ich würde sagen, FunctionEntry sollte diese Logik behandeln.
Das sollte nicht von Configure() getan werden.
Ich denke, es sollte die ItemType der ButtonEventArgs und die IBrkoer an die Instanz FunctionEntry übergeben und lassen Sie diese if-else Logik entscheiden.

Soweit die verschachtelten if-else, bin ich nicht allzu besorgt darüber.
Business-Logik kann so komplex sein, wie sie will, besonders wenn es an Einheitlichkeit mangelt.
Es wäre komplizierter, eine Nachschlagetabelle für diese Logik zu erstellen, da Sie in diesem Fall eine 3-dimensionale Tabelle benötigen: welche Schaltfläche, welche Broker-Handelsphase und welcher Parameter geändert werden soll. Das wäre eine Hektik, der man folgen sollte, um ehrlich zu sein.

Answer 3

Damit eine Strategie angewendet werden kann, muss sie sich in dem Objekt befinden, das über die Daten verfügt, die die Strategie auswählen. Sie ziehen Daten aus dem Broker, der Handelsphase des Brokers und einer Schaltfläche.

Um diese Kombination mit Polymorphismus zu machen, ist eine dreifache Verteilung erforderlich, und wäre weitaus komplizierter als der aktuelle Code.

Sie können nach Phase trennen und Demeter anwenden.

Es gibt auch viele magische Zahlen. Wenn Sie von einem System in ein anderes übersetzen, sollten Sie sie zu Konstanten des zu übersetzenden Systems machen, andernfalls verschieben Sie sie in Ihr Datenmodell.

Answer 4

Es ist schwer zu sagen, ohne mehr über Ihren Code zu wissen, aber die Probleme sieht aus, als ob Sie versuchen, mehr als eine Sache in der Methode zu tun, auch Code abhängig von der Benutzeroberfläche in einer Datenmethode wie das sieht ziemlich hässlich aus .

Es scheint, wie Sie mindestens zwei Methoden, ein GetBrokerPrice und ein SetPriceOptions

Answer 5

Zwei offensichtliche Refactorings in den Sinn kommen sollte:

1. entfernen die else: jetzt die ifs neu angeordnet werden können leichter.
2. Ordnen Sie die if's so, dass if (phase == ...) immer die äußere ist.

Wenn Sie möchten, können Sie die if-Blöcke neu anordnen, um die if (phase == 1) Blöcke zu verbinden, obwohl ich glaube, ich würde wiederholen, dass if (phase == 1) mehrere Male für den nächsten Schritt vorzubereiten.

Diese Refactorings erleichtern die Anwendung der folgenden.

internal void Configure(ButtonEventArgs args, IBroker broker, FunctionEntry entry) 
{ 
     int phase = broker.TradingPhase; 

     if (phase == 1) 
     {        
      if (args.Button == ItemType.SendAutoButton) 
      { 
       entry.SetParameter("ANDealerPrice", -1); 
       entry.SetParameter("ANAutoUpdate", 4); 
      } 
     } 
     if (phase == 2) 
     {        
      if (args.Button == ItemType.SendAutoButton) 
      { 
       entry.SetParameter("ANDealerPrice", -1); 
       entry.SetParameter("ANAutoUpdate", 2); 
      } 
     } 
     if (phase == 1) 
     { 
      if (broker.IsCashBMK) 
      { 
       entry.SetParameter("Value", 100); 

      } 
     } 
     if (phase == 1) 
     { 
      if (broker.IsCross) 
      { 

       entry.SetParameter("Value", 200); 

      } 
     } 

}

Jetzt haben Sie eine lange Liste von kleinen if-Blöcke. Dies kann in eine List<MyAction> umgestaltet werden. Irgendwo müssen Sie diese Liste füllen, aber es ist ziemlich einfach durchqueren:

internal void Configure(ButtonEventArgs args, IBroker broker, FunctionEntry entry) 
{ 
     foreach(var action in MyActions) 
     { 
      action(args, broker, entry); 
     } 
} 
internal void PopulateMyActions() 
{ 
     // Hopefully I have not made a syntax error in this code... 
     MyActions.Add((ButtonEventArgs args, IBroker broker, FunctionEntry entry) => 
     { 
      if (broker.TradingPhase == 1) 
      {        
       if (args.Button == ItemType.SendAutoButton) 
       { 
        entry.SetParameter("ANDealerPrice", -1); 
        entry.SetParameter("ANAutoUpdate", 4); 
       } 
      } 
     }); 
     // And so on 
} 

Eine Alternative ist, separate Listen für Phase == 1 und Phase == 2, zu erstellen und den Broker aus dem Aufruf auslassen zu action :

internal void Configure(ButtonEventArgs args, IBroker broker, FunctionEntry entry) 
{ 
     int phase = broker.TradingPhase; 
     foreach(var action in MyActions[phase]) 
     { 
      action(args, entry); 
     } 
} 

internal void PopulateMyActions() 
{ 
     // Hopefully I have not made a syntax error in this code... 
     MyActions[1].Add((ButtonEventArgs args, FunctionEntry entry) => 
     { 
      if (args.Button == ItemType.SendAutoButton) 
      { 
       entry.SetParameter("ANDealerPrice", -1); 
       entry.SetParameter("ANAutoUpdate", 4); 
      } 
     }); 
     // And so on 
} 

ich glaube, ich letzteres bevorzugen, da es die besondere Rolle der phase deutlicher machen.

Zusätzliche Refactorings könnte action(args, entry) mit action(args.Button, entry) ersetzen, aber ich kann nicht beurteilen, ob das angemessen ist.

In Zukunft könnte das Auffüllen der Liste dynamisch erfolgen, z. beim Laden einer Baugruppe. Welche Baugruppe geladen werden soll, könnte dann durch eine Konfigurationseinstellung gesteuert werden. Presto: Wechseln Sie das Verhalten, ohne Ihren Kerncode neu zu kompilieren!

PS: Code ist nicht getestet, da ich gerade von einem Compiler entfernt bin. Fühlen Sie sich frei, meine Antwort zu bearbeiten, um Syntaxfehler zu entfernen, die Deklaration von MyActions hinzuzufügen, und so weiter.