if-statement - Arrow Anti-Pattern のリファクタリングのアイデアが必要

Question

モンスターを受け継いだ。

これは、.NET 1.1 アプリケーションが Healthcare Claim Payment (ANSI 835) 標準に準拠するテキストファイルを処理するように見せかけていますが、怪物です。処理される情報は、医療請求、EOB、および償還に関連しています。これらのファイルは、最初の数桁に識別子を持つレコードと、そのタイプのレコードの仕様に従ってフォーマットされたデータフィールドで構成されます。一部のレコード ID は、特定のタイプのトランザクションに関連するレコードのグループを区切る制御セグメント ID です。

ファイルを処理するために、私の小さな怪物は最初のレコードを読み取り、これから行われるトランザクションの種類を判断し、現在処理しているトランザクションの種類に基づいて他のレコードの処理を開始します。これを行うには、ネストされた if を使用します。多数のレコードタイプがあるため、いくつかの決定を下す必要があります。各決定には、いくつかの処理と、以前の決定に基づいて行う必要がある 2 ～ 3 のその他の決定が含まれます。つまり、ネストされた if には多くのネストがあります。そこに私の問題があります。

この入れ子になった if の長さは 715 行です。はい、そうです。七百五十行。私はコード分析の専門家ではないので、フリーウェアの分析ツールをいくつかダウンロードし、McCabe の Cyclomatic Complexity 評価で 49 という結果になりました。彼らは、これはかなり高い数値だと言っています。100 が高い基準であるアトランタ地域の花粉数のように高く、ニュースは「今日の花粉数は 1,523 です」と言っています。これは、私が今までに見た中で最高の矢印アンチパターンの例の 1 つです。インデントは最大で 15 タブの深さになります。

私の質問は、そのようなものをリファクタリングまたは再構築するためにどのような方法を提案しますか?

アイデアを探すのにしばらく時間を費やしましたが、良い足がかりが得られませんでした。たとえば、レベルをガード条件に置き換えることも方法の 1 つです。私はそれらのうちの1つだけを持っています。巣が 1 つ落ちて、残りは 14 です。

参考になるデザインパターンがあるかもしれません。コマンドチェーンはこれにアプローチする方法でしょうか? .NET 1.1 のままにしておく必要があることに注意してください。

ありとあらゆるアイデアをありがとう。

score 20 · Accepted Answer

今週、あなたが説明しているものと似た（悲惨ではありませんが）レガシーコードがいくつかありました。

これからあなたを得るものは1つもありません。ステートマシンは、コードがとる最終的な形式かもしれませんが、それはそこに到達するのに役立ちません。また、既に発生している混乱を解く前に、そのような解決策を決定する必要もありません。

最初のステップは、既存のコードのテストを作成することです。このテストは、コードが正しいことを示すためのものではなく、リファクタリングを開始するときに何かが壊れていないことを確認するためのものです。処理する大量のデータを取得し、それをモンスターに供給して、出力を取得します。それがあなたのリトマス試験紙です。コードカバレッジツールを使用してこれを行うことができれば、テストでカバーされていないものが表示されます。可能であれば、このコードを実行するいくつかの人為的なレコードを作成し、繰り返します。このタスクでできることを完了したと感じたら、出力データはテストの期待される結果になります。

リファクタリングによってコードの動作が変更されることはありません。それを覚えておいてください。これが、物事を壊さないことを検証するための既知の入力データセットと既知の出力データセットがある理由です。これがあなたのセーフティネットです。

今リファクタリング！

私が行ったいくつかのことは、私が役に立つと思ったものです:

ifステートメントを反転

私が抱えていた大きな問題は、対応するステートメントが見つからないときにコードを読むことでしelseた.多くのブロックがこのように見えることに気付きました.

if (someCondition)
{
  100+ lines of code
  {
    ...
  }
}
else
{
  simple statement here
}

を反転するifことで、単純なケースを確認してから、他のブロックが既に行ったことを知っているより複雑なブロックに進むことができました。大きな変化ではありませんが、理解するのに役立ちました。

抽出方法

私はこれをよく使いました.いくつかの複雑な複数行ブロックを取り、それを理解し、それを独自の方法で押しのけます. これにより、コードの重複がある場所をより簡単に確認できるようになりました。

これで、コードが壊れていないことを願っています (テストはまだパスしますか?) 。手続き型コードがより読みやすく、よりよく理解されています。すでに改善されていることを確認してください。しかし、あなたが以前に書いたそのテストは本当に十分ではありません...元のコードの機能（バグとすべて）を複製していることを伝えるだけで、それはあなたがカバーしていた行だけです.ヒットする方法がわからない、またはヒットできないコードブロックを見つけるでしょう（私は両方とも私の仕事で見ました）。

すべての有名なパターンが登場する大きな変化は、これを適切なオブジェクト指向の方法でリファクタリングする方法を検討し始めたときです。この猫の皮を剥ぐには複数の方法があり、複数のパターンが含まれます。解析しているこれらのファイルの形式に関する詳細がわからないので、最善の解決策である場合とそうでない場合があるいくつかの役立つ提案を投げかけることしかできません。

Refactoring to Patternsは、このような状況で役立つパターンを説明するのに役立つ優れた本です。

あなたは象を食べようとしていますが、一度に一口ずつ食べる以外に方法はありません。幸運を。

score 2 · Accepted Answer

Extract Method を無制限に使用することから始めます。現在の Visual Studio IDE にない場合は、サードパーティのアドインを取得するか、プロジェクトを新しい VS に読み込むことができます。(プロジェクトをアップグレードしようとしますが、それらの変更をチェックインするのではなく、慎重に無視します。)

コードを 15 レベルインデントしたとおっしゃいました。約 1/2 ウェイアウトから開始し、メソッドを抽出します。いい名前が思いつかればそれを使い、うまくいかなければ抽出してください。さらに半分に割る。ここでは理想的な構造を目指しているわけではありません。コードを自分の脳に収まるような断片に分解しようとしているのです。私の脳はそれほど大きくないので、痛くなくなるまで壊して壊し続けます.

作業を進めながら、残りのメソッドとは異なると思われる新しい long メソッドを探します。これらを新しいクラスにします。今のところ、メソッドが 1 つしかない単純なクラスを使用してください。メソッドを静的にすることは問題ありません。彼らが良いクラスだと思うからではなく、あなたが何らかの組織に必死になっているからです.

作業中に頻繁にチェックインすることで、作業のチェックポイントを設定し、後で履歴を理解し、マージを必要とせずに「実際の作業」を行う準備ができ、チームメイトが困難なマージの手間を省くことができます。

最終的には、戻ってメソッド名が適切であること、作成したメソッドのセットが意味を成していること、新しいクラスをクリーンアップすることなどを確認する必要があります。

信頼性の高い Extract Method ツールを使用している場合は、優れた自動テストがなくても問題ありません。(たとえば、これに関しては VS を信頼します。) そうしないと、何かを壊していないことを確認してください。

ここでは、ペアリングパートナーが役に立ちます。

score 2 · Accepted Answer

このような場合に私が行うことの 1 つは、'Composed Method' パターンを使用することです。このテーマについては、 Jeremy Miller のブログ投稿を参照してください。基本的な考え方は、IDE のリファクタリングツールを使用して小さな意味のあるメソッドを抽出することです。それができたら、さらにリファクタリングして意味のあるクラスを抽出できるかもしれません。

score 2 · Accepted Answer

ステートマシンは開始する論理的な場所のように思われ、それをスイングできる場合は WF を使用します (できないように聞こえます)。

WF を使用せずに実装することもできますが、自分で行う必要があります。ただし、最初からステートマシンのように考えると、すべてのアクションで内部状態をチェックするプロシージャルモンスターを作成するよりも、より適切な実装が得られるでしょう。

遷移の原因となる状態を図解します。レコードを処理する実際のコードは分解して、状態の実行時に呼び出す必要があります (特定の状態が必要な場合)。

したがって、State1 の実行は「レコードの読み取り」を呼び出し、そのレコードに基づいて別の状態に遷移します。

次の状態では、複数のレコードを読み取り、レコード処理命令を呼び出してから、状態 1 に戻ります。

score 1 · Accepted Answer

説明から判断すると、ステートマシンが最適な方法である可能性があります。現在の状態を格納する列挙型変数を用意し、現在の状態と入力データに基づいて実行するアクションを選択する switch または if ステートメントを使用して、レコードのループとして処理を実装します。作業が大きくなりすぎた場合は、関数ポインターを使用して、状態に基づいて別の関数に作業を簡単にディスパッチすることもできます。

score 1 · Accepted Answer

Coding Horror に、これに関する非常に優れたブログ投稿がありました。私はこのアンチパターンに一度だけ遭遇したことがあり、ほとんど彼の手順に従っただけです.

score 1 · Accepted Answer

状態パターンとスタックを組み合わせることもあります。

階層構造に適しています。親要素は、子要素を処理するためにスタックにプッシュする状態を知っていますが、子要素はその親について何も知る必要はありません。言い換えれば、子は次の状態が何であるかを知りません。それは単に「完了」したことを通知し、スタックからポップされます。これは、依存関係を一方向に保つことで、状態を互いに分離するのに役立ちます。

これは、SAX パーサーを使用して XML を処理するのに最適です (コンテンツハンドラーは、要素が入力および終了されるときに、状態をプッシュおよびポップして動作を変更するだけです)。EDI もこのアプローチに役立つはずです。

if-statement - Arrow Anti-Pattern のリファクタリングのアイデアが必要

7 に答える 7

Related

Reference