Zenn
🦁

レガシーコード改善ガイドをざっくり読んでみた

に公開
テスト駆動開発
レガシーコード
unitテスト
tech

最初に

レガシーコード改善ガイドを読んでいる際に、個人的につけていたメモです。

いわゆるまとめ記事ではなく、内容も飛び飛びで自分の感想もりもりですが、参考になれば。

https://amzn.asia/d/bPzxhbM

第1部　変更のメカニズム

単体テストの定義

実行に0.1秒もかかる単体テストは遅い単体テストであり、遅い単体テストはもはや単体テストではない。データベースとやり取りしたり、ファイルシステムにアクセスするテストは単体テストではない。と述べられていたが、「単体テストの考え方/使い方」では、モックをなるべく使わずテスト用DBへ接続したほうが質の高いテストが作成できると書かれていたことを思い出した。

テストが高速で実行できることは素晴らしいことだが、そのためにモックを多用し、何をテストしているのかわからないテストになってしまうのも良くないので、バランスむずいね～と思った。要はバランスおじさん。個人的には「単体テストの考え方/使い方」の主張の方がしっくりくる。実際のDBつなぐ前提にしていかにして速度的にも品質的にも安定した単体テストを書いていくかを考えるほうが良い気がしないでもない。

有名なレガシーコードのジレンマ「コードを変更するためにテストを整備したいが、テストを整備するためにコードを変更しなければならない」の原典を読めた。どうやって解決していくかはもう少し後ろの章にある模様。

レガシーコードの変更手順
  1. 変更点を洗いだす
  2. テストを書く場所を見つける
  3. 依存関係を排除する
  4. テストを書く
  5. 変更とリファクタリングを行う

やはり一番キモとなるのは依存関係を排除すること。依存関係を排除するための変更によって振る舞うが壊れていないことをテストできるのが理想だが多くの場合はそうではない。そんな中でのベストプラクティスについては後述する章に書いてある。

偽装オブジェクト

あるクラスをテストする際に依存関係を持つ協調クラスをテスト用の偽装オブジェクトに置き換えることで、テストを行いやすくするテクニックが紹介されていた。その過程の中でクラスAがクラスBに直接依存している部分をインターフェースに依存する形にして、偽装オブジェクトを外からクラスAに流し込むように変更している例が書かれていた。これはまさしく依存性逆転の原則の典型例だった。本の中でも述べられていたが、偽装オブジェクトを用いたテストはモックでおおむね代替できるので、頻繁に使えるテクニックではないかもしれないが、ためになった。

接合部と許容点

  • 接合部（Seam）

    • 「既存コードの振る舞いを差し替え・変更できる“境界”」のこと。
    • 例）外部呼び出しを行うメソッド呼び出し箇所、new で生成しているオブジェクト、リンク時の関数差し替え など

  • 許容点（Enabling Point / Enabling Seam）

    • その接合部で「どちらの振る舞いを使うか」を最終的に切り替える“スイッチ”や仕組み のこと。
    • 「この箇所でモックを呼ぶのか本物の処理を呼ぶのか」を選択できるようにするポイントのこと。

第2部　ソフトウェアの変更

スプラウトメソッド（Sprout Method）
  • 既存メソッドを大きく修正せず、新しいメソッド（“新芽”）を生やして新機能を実装する手法
  • 既存部分の変更は最小限に留め、追加機能はスプラウトメソッド側に集中させる
  • 新しいメソッドが独立しているため、テストがしやすい
  • 既存コードの根本的な問題や複雑さを直接解消するわけではない
ラップメソッド（Wrap Method）
  • 既存の処理を“包み込む（wrap）” 新しいメソッドを用意し、そこに追加ロジックやテストフックを挿入する手法
  • 元の関数やメソッドにほとんど触れずに、前後や周囲の処理を拡張する形で新たな振る舞いを組み込む
  • 呼び出し側を段階的にラップメソッドに置き換えていく
  • やはり既存のレガシー部分を根本的に取り除くわけではない

どちらも既存メソッドに極力手を入れずに新しく機能を追加する方法となる。一時的には汚いコードができるかもしれないが、繰り返すことで徐々にレガシーなコードから脱却していく。

一方、テストしていない範囲と重複するコードを追加してしまっている場合、重複が残り続けることとなり、結果バグの原因を引き起こす可能性も持っている。

そのため、既存のコードにどうにかテストを書いて、新しい機能を追加しやすくするアプローチをとることが本来は望ましい。そして、このための手法としてテスト駆動開発があげられていた。

レガシーコードに対するテスト駆動開発の流れ
  1. 変更したいクラスをテストで保護する
  2. 失敗するテストケースを記述する
  3. 実行して失敗することを確認する
  4. テストを通過できるように実装をする
  5. テストがパスする状態を保ちながら重複を取り除く

変更したいクラスをテストで保護する、ということろがミソで、どのような観点でテストするのかというあたりの付け方を間違えるとバグを混入しかねないなぁと思った。

DIの匙加減の話

テストしやすくするためにテスト対象のオブジェクト内で生成されている別のオブジェクトを外部から注入する形にすることで、テスト用のオブジェクトにすり替えやすくなるよ、可読性も上がるよというテクニックが頻出していて、実際有用である一方でさじ加減どうするべきなのだろうとも思った。ということをChatGPTに聞くと以下の内容が返ってきて腑に落ちた。

(1) テスト時にモック化・スタブ化したいか？
  • 「この依存を差し替えないとユニットテストが難しい」と思うなら、外部注入が基本
  • 「テストに関係なく、依存も外部リソースもない」のなら内部生成でも問題ない
(2) 初期化コストや外部リソースの存在
  • DB接続やネットワークコネクション、設定ファイル読み込みなどがあるなら外部注入
  • 何もなくて素直に new ClassA() だけで済み、動作も軽いなら内部生成で十分
(3) 実装の将来拡張性
  • 「もし別実装に切り替えがあるかもしれない」「将来の改修で差し替えが多そう」→ インターフェース経由の外部注入が安全
  • その機能が固定で、他にあり得ないなら内部生成でも過剰にはならない
プライベートメソッドのテスト

プライベートなメソッドをパブリックにしてまでテストせず、パブリックメソッドを経由してテストできるならその方が望ましい。その一方で、プライベートなメソッドをテストしたいならパブリックメソッドにしてテストしよう、なぜならテストしたいということは単一責任の原則に違反しているような複雑なメソッドになっている可能性が高いからである。また、パブリックメソッドにすることで想定されるリスクは結局テスト対象のメソッドを新しいクラスに切り出して別個でテストすれば解決する、というのも納得だった。確かにその通り。

絞り込み点

絞り込み点とはテストコードを実装することで、依存関係を持つ各メソッドに対する変更による不具合を検出できる場所のことを指す。例えば、メソッドAとメソッドBがそれぞれ別のクラスで実装されており、メソッドCから各メソッドを用いている場合、メソッドCに対するテストコードを実装することで、メソッドAとメソッドBの変更によってバグが作成された際の不具合を検知することができるようになる。言ってしまえばテストコードを書くことのコスパが良いところともいえる。とはいえ絞り込み点にテストコードを書くということは少なからず粒度が荒い結合テストチックなテストコードを書くことに近づいていくため、乱用には注意が必要。十分なテストコードがない状況において、後退に対する保護を行うためのテストとして理解し、徐々に各クラスそれぞれにターゲットしている単体テストを別途実装して保護していくことが望ましいのだろう。

仕様化テスト

リファクタリング前後での品質担保のためにはリファクタリング対象のコードの振る舞いが変更されていないことを確認するテストを実装してからリファクタリングをする、という手順を踏むことが必要である。

既存のコードの振る舞いを上手に理解するためには、コードリーディング、過去のドキュメントなどがあるが、加えてテストを書きながら仕様を理解していく、というアプローチも存在する。

ひとつひとつのテストケースを実装していきすべてのパターンを網羅するのは不可能であるため、あくまでコードの振る舞いを理解するために実装するテストケースとなるが、それらは対象のコードの振る舞いを表現する非常に信頼できるドキュメントと化す。

責務の分離

クラスを分割する際に、既存のクラスがどのような責務を抱えているかを分析する必要がある。分析の手法としてメソッドを羅列して、属性を把握する「メソッド分類法」があげられていた。ほかにもいくつか挙げられていたが、各メソッドが度のインスタンス変数を呼び出しているかを図示化してグルーピング化する「機能スケッチ」は単純だがかなり端的に分類できる手法だと感じた。

単一責任の原則

単一責任の原則に違反するパターンとして１．インターフェースレベルでの違反と２．実装レベルの違反が存在する。1の場合は、呼び出し側で関心があるメソッドの身を別クラスとして切り出してFacadeパターン的な状態に変更することで改善でき、2の場合はいくつかのクラスに責務を委譲した状態にすることで一定改善が見込まれる。

重複部分を取り除くリファクタリング

同じ抽象クラスから派生した具象クラスがいくつかある場合、時間の経過とともにコードの重複が発生する。重複している状態だと変更に対するコストが高いため、重複している部分を抽象クラスに切り出して具象クラスから重複部分を除去することが望ましい。安全に抽象クラスへの切り出しを行うためには既存の具象クラスの各メソッドに対してUnitテストが十分に実装されていることと、コードが構造化されていることが重要となりそう。各具象クラスのメソッドが構造化されていることで、抽象クラスに引き上げられそうなメソッドのあたりを付けやすくなるからだ。よって、抽象クラスにメソッドを引き上げるタイプのリファクタリングを行う際には事前準備として、既存の各具象クラスにUnitテストを実装し構造化を行っておく、ということが必要になりそうと感じた。

検出用変数の利用

モンスターメソッドをリファクタリングする場合、メソッドの状態を検出できるようにする方法として、検出用変数の導入がある。モンスターメソッドの内部の振る舞いをチェックするためのフラグ変数を導入して、特定のメソッドの実行前後でフラグの値が期待した値に変化しているかを追いかけることで比較的簡単に洗い粒度でのUnitテストを実装できる。

第3部　依存関係を排除する手法

インターフェースの抽出

依存を排除するためには、依存関係をクラスの外側から渡す形に変更することが有効であり、そのためにインターフェースの抽出が必要となる。これはすなわち既存のコードにおける依存性を逆転させることに他ならない。なるべくシグネチャを変えないようにする手順などいろいろ役立つテクニックが書いてあり役立ちそうだと思う一方で、依存性を逆転させるということは少なからず呼び出し側の実装も変更が必要であり、となると呼び出し側のメソッドなりクラスなりに対してもUnitテストを事前に準備しておく必要があるなぁと思った。さらに上位の呼び出し元があるとそこに対するUnitテストも書かなくてはならないのか？とも思ったがそこは手動テストでサクッとカバーするでも全然ありな気がした・・・

Discussion