4

先日のコードレビューでこんな感じのコードを見つけました。

public void DoSomeTasks()
{
  if (CheckSomeState()==true) return;
    DoTaskOne();
  if (CheckSomeState()==true) return;
    DoTaskTwo();
  if (CheckSomeState()==true) return;
    DoTaskThree();
  if (CheckSomeState()==true) return;
    DoTaskFour(); 
}

タスクの数が増えると、コードのサイクロマティックな複雑さがさらに増し、私には適切ではないと感じます。

これを解決するために私が思いついた解決策は次のとおりです。

private void DoTasksWhile(Func<bool> condition, Action[] tasks)
{
   foreach (var task in tasks)
   {
      if (condition.Invoke()==false) break;
        task.Invoke();
   }
}

このように使用

public void DoSomeTasks()
{
 var tasks = new Action[] { 
  {()=DoTaskOne()},
  {()=DoTaskTwo()},
  {()=DoTaskThree()},
  {()=DoTaskFour()}

  }

  DoTasksWhile(()=>CheckSomeState(), tasks);
}

コードを読みやすくするための提案はありますか?

4

3 に答える 3

4

私はあなたの実装を少しリファクタリングしました

private void DoTasksWhile(Func<bool> predicate, IEnumerable<Action> tasks)
{
    foreach (var task in tasks)
    {
        if (!predicate())
            return;
        task();
    }
}
  • Invoke委任する必要はありません。それらを実行するだけ
  • ブール値を比較しないでくださいtrue/false（役に立たず、誤ってブール値を割り当てることができます）
  • したがって、タスクを列挙するだけでIEnumerable、パラメーターに適しています

また、拡張メソッドを作成することもできます

public static void DoWhile(this IEnumerable<Action> actions,Func<bool> predicate)
{
    foreach (var action in actions)
    {
        if (!predicate())
            return;
        actions();
    }
}

使い方はとても簡単です:

tasks.DoWhile(() => CheckSomeState());
于 2012-10-27T17:32:11.487 に答える
1

オーケストレーション コードが非常に複雑になる場合は、WF (ワークフロー基盤) などのフレームワークの使用を検討してください。

それ以外の場合、コードは問題ありませんが、最初のコードの方が読みやすいので変更しません。また、将来的にこれらの IF を変更する可能性があるため、より柔軟です。つまり、条件が同じままではない可能性があります。

循環的複雑度がどのように低下​​するか、または低下できるかわかりません。

于 2012-10-27T17:31:45.683 に答える
0

現状ではソリューションは完全に適切ですが（4つのステップの場合はやり過ぎかもしれませんが）、この場合に最適なアプローチであるかどうかは、プログラムのコンテキストでCheckSomeStateの目的が実際に何であるかによって異なります。

  • CheckSomeStateがすべてのタスク間で1回だけ呼び出されることが重要ですか？
  • タスクの途中で呼び出す必要がある場合もありますか？

したがって、たとえば、CheckSomeStateが実際にキャンセルフラグをチェックしている場合、長時間実行されるタスク内で追加のチェックが必要になる場合があります。

使用可能な別のオプションは、例外をスローすることです（たとえば、OperationCancelledException）。これにより、循環的複雑度が非常に低く抑えられます。これにより、次のことが簡単にできるようになります。

CheckForCancel();
DoTaskOne();
CheckForCancel();
DoTaskTwo();

（欠点は、これが一般的に眉をひそめている制御フローの例外を使用していると考える人がいることです）。

また、目標は循環的複雑度を減らすことではなく、理解と保守が容易なコードを作成することであるべきだということも付け加えておきます。循環的複雑度は有用な指標ですが、非常に単純なコードに不必要にペナルティを課す場合があります。

于 2012-10-27T17:40:14.603 に答える