この記事は BASE アドベントカレンダー 2022 の 10 日目の記事です。
はじめに
初めまして、BASE のバックエンドエンジニアの shiiyan と申します。この記事では、ファットな注文検索モデルをリファクタしたことの経緯と感想について紹介します。
注文検索モデルをリファクタする理由
古典的な MVC モデルでは、スキニーコントローラー・ファットモデル（Skinny Controller, Fat Model）の考え方があるゆえに、プロダクトの成長と共に、モデルが肥大化していくことは多々あります。
BASE において、注文モデルと注文検索モデルはこの流れから逃れられませんでした。2022 年初めの時点では、注文検索を担当するモデルクラスは 1 ファイル 3000 行弱の規模に肥大化しています。こういったモデルクラスでは、コード自体が読みづらいだけでなく、機能改修時に影響範囲が読みにくく、メンテコストが高いという課題がありました。
2021 年 12 月から 2022 年 3 月まで、私は注文管理改善プロジェクトに参加していました。このプロジェクトではボトルネックを特定し、注文検索のパフォーマンス改善を目的としていました。ボトルネックを特定するために、まず処理の流れを理解する必要があり、そして、工数の見積りを出すために、注文検索処理を修正した場合の影響範囲を調査する必要がありました。こういった背景で、改修する前に、まず既存コードが分かりづらいという課題を解消することに合理性を感じ、プロジェクト内でリファクタリングの意思決定を行いました。
リファクタリングの意思決定
今年 3 月ごろに、注文検索周りの処理をリファクタすることについて、担当 PM と合意ができました。リファクタを進めるために、以下の前提があります。
- 進行中プロジェクトの進捗に影響しないこと
- 優先度が高いプロジェクトができた場合に、いつでもリファクタリングから切り替えられること
これらの前提を満たすために、以下のようなリファクタの進め方を計画しました。
- ボトムアップ方式
- テストファースト
- 毎朝 1 時間のペアプロ
ボトムアップ方式というのは、注文検索の既存処理を俯瞰してリファクタリング方針を決め、その後に順序的に進むことではなく、まずは変数名や関数名など些細なところからリファクタリングをし、少しずつ範囲を拡大していく方式です。この方式をとる理由は、条件が複雑な注文検索処理の全体を俯瞰し短い時間の中でリファクタリング方針を決めることが難しいと判断したためです。
テストファーストというのは、リファクタ対象のクラスやそのクラスの上位クラスに対してまずテストを追加し、リファクタをしても既存処理がデグレていないことを担保することです。これは、デバッグ時の生産性を高めることとリリース時のリグレッションテストのコスト削減を図ろうとしています。
毎朝 1 時間のペアプロをとる理由は、日中のプロジェクトの進捗に影響を出したくない、また、ペアプロすることにより、リファクタが目指す良いコードの基準を合わせたかったためです。
注文検索モデルの課題点
リファクタする前の注文検索モデルには以下の課題点がありました。
- コーディングスタイルが決まった前に実装したため、変数名と関数名にスネークケースとキャメルケースが混在していた
- 同じ変数名で再代入を重ね、変数の値を追うことが難しかった
- 200 行を超える関数が複数存在し、関数名も自明ではなかった
- 注文検索用のモデルだが、クラス名は注文モデルと区別ができなかった
- 複雑な検索条件の分岐を全て 1 箇所で対応したため、検索クエリを構築する処理が複雑化していた
リファクタリングのやり方
リファクタリングの手法や基準はおもに Martin Fowler さんの『リファクタリング 既存のコードを安全に改善する（第 2 版）』を参考にしました。これらの手法を PhpStorm という IDE の機能やプラグインと一緒に使うと効率よくリファクタリングを進められます。今回の注文検索モデルのリファクタリングで頻繁に利用したリファクタ手法をいくつか紹介します。
変数名や関数名をキャメルケースにする
BASE のコーディングスタイルを基準に、変数名と関数名をキャメルケースに統一します。PhpStorm デフォルト機能で Toggle case というものがありますが、変更したいケースに一発で変えられるようにしたいため、String Manipulation というプラグインをインストールしています。直前に変更したケースを記憶しているので、複数箇所連続でスネークケースからキャメルケースに変換する時に重宝しています。
変数の抽出と変数のインライン化
同じ変数名で再代入を重ね、変数の値を追うことが難しかったの課題を解消するために、変数の抽出と変数のインライン化を適用します。その時に、利用されるのが PhpStorm のリファクタ機能です。リファクタ対象を選択して、右クリックのメニューで Refactor | Introduce Variable を選ぶと変数の抽出ができます。Refactor | Inline を選ぶとインライン化ができます。
PhpStorm のリファクタ機能では以下の 4 パターンの変数抽出に対応しております。
- ローカル変数
- コンスタント
- フィールド変数
- パラメーター変数
関数の抽出と関数のインライン化
200 行を超える関数が複数存在し、関数名も自明ではなかった課題を解消するために、処理の意図が伝わるまでに関数の抽出とインライン化を繰り返しました。関数の抽出はを Refactor | Extract Method 利用し、インライン化は Refactor | Inline を利用します。Extract Method は関数の抽出後、いい感じの関数名を提示してくれるだけでなく、複数箇所で同じ処理が書かれている場合に、全て対応してくれるのでとても便利です。
クラス名、関数名、変数名の変更
注文検索用のモデルだが、クラス名は注文モデルと区別ができなかった課題を解消するために、クラス名を OrderSearcher に変更しました。Refactor | Rename を利用します。クラス名を変更する時に 1 つ注意点があります。クラスを変更した時に、そのテストクラスの名前は変更されないことです。例えば、OrderSearcher を NewOrderSearcher に名前を変えても、OrderSearcherTest の名前はそのままです。追加でテストクラスに対して、 Refactor | Rename を使い名前を変更するようにしています。
不要な use の削除
注文検索モデルに CakePHP 2 依存の App::uses がたくさんあったため、それを PHP の use キーワードで名前空間を利用するようにしました。そうすると、PhpStorm の Code | Optimize Imports 機能を利用して、不要な use の削除ができます。さらに、Optimize Imports を Before Commit フックに入れると、PhpStorm 経由でコミットする前に、自動で import をチェックし、修正してくれるようになります。
ポリモーフィズムで注文検索の条件分岐をシンプルにする
複雑な検索条件の分岐を全て 1 箇所で対応したため、検索クエリを構築する処理が複雑化していた課題を解消するためにポリモーフィズムを導入しました。条件分岐の中で最も複雑なのは、検索対象の中にコイン注文（ショップコインという BASE 内かつて流通していた仮想通貨を使って購入した注文）が含まれているかどうかの分岐です。コイン注文ありなしの 2 パターンで検索クエリ構築、DB へ問い合わせ、返り値の整形という一連の流れを 1 つのクラス内で実装していたため、条件の分岐が散らばり、処理の重複も発生するようになりました。
注文検索モデルのコイン注文ありなしの分岐に対して、以下のリファクタリングを行いました。
- 同じインターフェイスを実装したクエリ生成クラスを別々に作成した。クエリ生成の共通処理は trait に切り出した。
- コイン注文ありなしの条件に応じて正しいクエリ作成クラスを生成するファクトリーを作成した。
- 注文検索モデルでは 2 のファクトリーを利用してクエリ構築の条件分岐を隠蔽した。
今回のリファクタから学んだこと
よかったこと
まずはリファクタすること自体がよかったです。既存コードに手を加えることにより、実装者の意図が伝わり、ドキュメントに書かれていない仕様の細かい部分への理解も深まりました。リファクタリングの結果、ソースコードの可読性とメンテナンス性が向上し、注文検索改善など後続プロジェクトが軽い身でスタートできるようになりました。今は、リファクタして損はないと考えています。
テストファーストとリファクタリングの相性がよかったです。テストを予め定義することで、このリファクタでデグレっているのかのフィードバックを最短でもらえるため、バグが発生しても早めに気づいて修正することができ、リファクタの生産性向上に貢献が大きかったです。今回のリファクタで 10 回弱のリリースの内、注文検索の全パターンを網羅的に手動テストしたのは最後の 1 回でしかないです。リリースして半年が経っても、バグが出ていませんでした。
ボトムアップ方式でリファクタの意思決定の合意が取りやすかったです。リファクタリングの合意がうまくいかないほとんどの場合は、進行中の開発タスクを優先したいと思います。ボトムアップ方式ならリファクタのための前期設計と計画が不要となり、いつでもリファクタリングを開始や停止にでき、いつでも開発タスクに戻れる状態が作れやすいです。
よくなかったこと
ボトムアップ方式に限界がありました。変数名の変更や、関数の抽出など局所的なリファクタでは設計せずにスムーズに進めましたが、ポリモーフィズムなどクラス全体あるいは、クラスを跨ぐ影響が出るリファクタでは予想していなかった理由でリファクタが進めなくなると手戻りコストが高いです。注文検索モデルの例では、元々はクエリ構築、DB へ問い合わせ、返り値の整形という一連の処理に対して、ポリモーフィズムを適応しクラスを分けようとしましたが、DB へ問い合わせは Model を継承したクラスでなければいけないというフレームワークの制限に引っかかり、2 回ほどやり直しが発生しました。手戻りコストが高いリファクタリングを行う前に、一定程度でトップダウン方式で設計しておいたほうが良いと思いました。
おわりに
本記事では、ファットな注文検索モデルをリファクタした実践について記載いたしました。読んでいただいた方の助けに少しでもなれば嬉しいです。
明日は @izuhara さんの記事が公開予定です。ぜひご覧ください！
