コードに「抽出メソッド」リファクタリングを適用する方法

Question

1200行のメソッド（スレッドの実行メソッド）を持つ従来のアプリケーションがあります。この方法は、文の長いシーケンスを含む単一の（真の）ものである。

次のC＃の領域は、この方法で約50倍存在する：

#region Cancel pending 
    if (backgroundWorkerPrincipal.CancellationPending) 
    { 
     if (CanCancelThread) 
     { 
      ev.Cancel = true; 
      return; 
     } 
    } 
#endregion 

I新しい方法には、この領域を抽出するための正しい（可能な場合）方法を思っています。

私が言ったように、その断片（領域）は、この方法の中で約50回出現する。 #region内での返品に注意してください（これは終了します）。

ので、この方法は、以下の構造を有する：

private void backgroundWorker_DoWork(object sender, DoWorkEventArgs ev) 

    while(true) { 

     ... 

     #region Cancel pending 
      if (backgroundWorkerPrincipal.CancellationPending) 
      { 
       if (CanCancelThread) 
       { 
        ev.Cancel = true; 
        return; 
       } 
      } 
     #endregion 

     ... 

     #region Cancel pending 
      if (backgroundWorkerPrincipal.CancellationPending) 
      { 
       if (CanCancelThread) 
       { 
        ev.Cancel = true; 
        return; 
       } 
      } 
     #endregion 

       ... 

     #region Cancel pending 
      if (backgroundWorkerPrincipal.CancellationPending) 
      { 
       if (CanCancelThread) 
       { 
        ev.Cancel = true; 
        return; 
       } 
      } 
     #endregion 

       ... 

     #region Cancel pending 
      if (backgroundWorkerPrincipal.CancellationPending) 
      { 
       if (CanCancelThread) 
       { 
        ev.Cancel = true; 
        return; 
       } 
      } 
     #endregion 

     ... 

     #region Cancel pending 
      if (backgroundWorkerPrincipal.CancellationPending) 
      { 
       if (CanCancelThread) 
       { 
        ev.Cancel = true; 
        return; 
       } 
      } 
     #endregion 

     . 
     . 
     . 

    } 

} 

Answer 1

私は、それをリファクタリングする正しい方法があることにそれを抽出するなどの動作しますそれを行うにはいくつかのファンキーな方法を、言わないだろう制御文を実行するかどうかをtrue/falseで返すメソッド。あなたはまだそれを50回繰り返さなければならないので、そうすることで得ることはあまりありません。

私は、このコードブロックをリファクタリングすることは一切勧めません。代わりに、私はあなたにリファクタリングコードの周りにを提案するつもりです。隣接するコードをリファクタリングすると、以前は識別できなかったパターンが表示されることがあります。

「...」ブロックのそれぞれについてメソッドを抽出することから始めます。あなたが今持っているのは、「メソッドを呼び出し、取り消しが保留中であれば救済する」というパターンです。これらのメソッドをデリゲートに変換することで、それらはデータ要素になり、ループすることができます。

抽出されたメソッドのシグネチャが同じであるとします。デリゲートインスタンスの配列を宣言し、ループでそれらを実行し、各繰り返しの最後に保留中のキャンセルを確認します。あなたは休憩の代わりにリターンを持っているので、あなたは内側のループから出るために余分なことをする必要はありません。

var extractedMethods = new Func<State, DoWorkEventArgs, State>[] 
{ 
    DoStep1, 
    DoStep2, 
    DoStep3, 
    // ... 
}; 

while (true) 
{ 
    foreach (Func<State, DoWorKEventArgs, State> fn in extractedMethods) 
    { 
     state = fn(state, ev); 

     if (backgroundWorkerPrincipal.CancellationPending && CanCancelThread) 
     { 
      ev.Cancel = true; 
      return; 
     } 
    } 
} 

「メソッドを呼び出した後、キャンセルが保留されている場合は救済」今、呼び出すために起こっているメソッドのリストから分離され、あなただけ維持するために1つの取消チェックを持っています。メソッドのリストが前面に確立され、そのブロックに供給されます。あなたは、独自のメソッドにwhileループを抽出してから、代理人のリストを渡す余分な手順を取ることができます。一方、それはあなたの必要にあまりにも遠すぎるかもしれません。

抽出されたメソッドのシグネチャが異なる場合、単純ではありませんが、いくつかのオプションがあります。メソッドを調整して同じパラメーターを使用し、使用しないパラメーターを無視させることができます。しかし、パラメータが多すぎると保守性が損なわれることがあります。特に50種類の方法を調整する必要がある場合は、特にそうです。将来パラメータのバリエーションが増えることが予想される場合、これは良い選択ではないでしょう。

もう1つの選択肢は、比較的シンプルな署名でlambdasを使用し、相違点を抽象化するためにクロージャを利用することです。

var extractedMethods = new Func<State, DoWorKEventArgs, State>[] 
{ 
    (st, ev) => RunStep1(st, ev /*, parameters specific to RunStep1 */), 
    (st, ev) => RunStep2(st, ev /*, parameters specific to RunStep2 */), 
    (st, ev) => RunStep3(st, ev /*, parameters specific to RunStep3 */), 
    // ... 
};