2010-11-20 15 views
3

異なる変数に基づく:は私のコードをリファクタリング:条件は、与えられた

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

       } 
      } 
} 

私は上記のコードをリファクタリングするための提案を探しています。 Fowler氏が提案したように、「条件をストラテジー/ポリモーフィズムに置き換える」とすると、これらの行に有効なコードを実装できません。複数の条件があるので、複数の変数に基づいています。

これらのエラーが発生しやすく、醜い条件(コードの匂い)を排除できるパターンがあるかどうかをご提案ください。

ありがとうございます。

編集: 1)ここではOpen-Closedの原理を適用することを意図しているので、もし論理が変更されていれば新しいクラスを導入することでこれらの条件を拡張することができます。 2)実際のシナリオでは、マジックナンバーを気にしないでください。有効な定数/ソースがあります。

答えて

2

あなたがこれまでに提示したもの、私はこのように、三つのパラメータ、独自の関数にそれぞれを分離するために傾けられる場合:

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

これはやや手作り(ある向いていませんルールの変更に伴い半自動化された更新に至るまで)、わずかに効率が悪い(いくつかの条件がチェックされ、いくつかのフィールドが複数回参照され、さらに多くの関数呼び出しが追加されます)。私は、実証済みの問題が発生するまでは効率は重要ではないと考えています(そうした時には、より大きな変更が必要になります)。このアプローチでは、特定のパラメータのルールが変更されたときにどのコードを調べるかを正確に知ることができます。私は多形性が良い解決策にあなたを導く可能性が高いとは思わない。

+0

+1は、ソリューションの簡素化(および可読性)のためです。これまでのところ、これは私にとって最も受け入れられる答えです。ありがとう。 –

1

文字列のパラメータをFunctionEntryに設定しているように見えます。
私はFunctionEntryはそのロジックを処理する必要がありますと言うでしょう。
これは、Configure()で行うべきではありません。
ButtonEventArgsItemTypeIBrkoerFunctionEntryのインスタンスに渡して、このif-elseロジックを決定させるべきだと思います。

ネストされたif-elseの限り、私はあまり心配していません。
ビジネスロジックは、特に統一性が欠けている場合は、必要なだけ複雑にすることができます。
このロジックのルックアップテーブルを作成すると、3次元テーブルが必要になるため、より複雑になります。どのボタン、どのブローカー取引フェーズ、そしてどのパラメータを変更するかです。それは正直なところ、従うのが面倒です。

+0

お返事ありがとうございます。 FunctionEntry(これは別のdllにあります)ではこのロジックを動かすことはできませんが、汎用インフラストラクチャレベルの場合は特定のシナリオとFunctionEntryにのみ適用されるためです。 –

2

ストラテジを適用するには、ストラテジを選択するデータを持つオブジェクトに配置する必要があります。あなたは、ブローカー、ブローカーの取引フェーズ、およびボタンの両方からデータを引き出しています。

多態性との組み合わせを行うには、トリプルディスパッチが必要であり、現在のコードよりはるかに複雑です。

フェーズごとに分離し、Demeterを適用したい場合があります。

マジックナンバーもたくさんあります。あるシステムから別のシステムに変換する場合は、システムの定数を変換するか、それ以外の場合はデータモデルに移動します。

2

あなたのコードについてもっと知ることなしに言うのは難しいですが、その方法で複数のことをしようとしているように見えます。また、コードはUIメソッドに依存しています。 。

あなたが、少なくとも二つの方法、1 GetBrokerPriceと1 SetPriceOptions

2

つを持つべきであるように明白なリファクタリングが頭に浮かぶようだ:

  1. elseを削除します。今、IFSは、より簡単に再配置することができます。
  2. if (phase == ...)が常に外側になるようにifを並べ替えます。

あなたがしたい場合は、私はif (phase == 1)複数回、次のステップのために準備することを繰り返すことと思いますが、if (phase == 1)ブロックを組み合わせることであれば、ブロックを並べ替えることができます。

これらのリファクタリングにより、以下のようなリファクタリングが簡単に適用できます。

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

      } 
     } 

}

今あなたが小さな場合、ブロックの長いリストを持っています。これは、List<MyAction>にリファクタリングすることができます。どこかで、あなたはこのリストを移入する必要がありますが、それを横断することは非常に簡単です:

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 
} 

代替は、位相== 1と位相== 2のための別々のリストを作成し、呼び出しから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 
} 

phaseの特別な役割をより明確にするため、私は後者を好むと思います。

追加のリファクタリングはaction(args, entry)action(args.Button, entry)に置き換えることができますが、それが適切かどうかは判断できません。

今後、リストを作成することは動的に行うことができます。アセンブリをロードするとき。どのアセンブリをロードするかは、構成設定によって制御できます。 Presto:コアコードを再コンパイルせずに動作を切り替えましょう!

PS:現時点でコンパイラから離れているので、コードはテストされていません。私の答えを編集して構文エラーを削除し、MyActionsなどの宣言を追加してください。

+0

+1あなたが提案したアプローチ。私はこの解決策を考えていませんが。 1)IMHO、ラムダ式は、読みやすく簡単なコードになると良い評価をしません。 2)各表現の条件を繰り返すことは良い解決策のようには思えない。ご回答有難うございます。 –

関連する問題