この記事はLITALICO Advent Calendar 2024のカレンダー2の21日目の記事です
https://qiita.com/advent-calendar/2024/litalico
はじめに
株式会社LITALICOでWEBエンジニアをやっている @ti_aiuto です。
普段は主に個人向けのWEBサービスの開発のサポートを担当していて、特にモノリスなアプリケーションを持続可能な形に保つことや、データモデリング・データ分析に関わる諸々の整備に関心があります。
背景
自分が担当しているLITALICO発達ナビというプロダクトには、児童発達支援事業所や放課後等デイサービスといった施設を検索する「施設情報」という機能があります。
よくある飲食店や不動産の検索の発達支援施設版だと思ってもらえれば良いと思います。
昨年度からこの機能のグロースに大きく注力していくことになり、様々な改善を日々行っています。
課題
その中で、大きく次の2つの課題が見えてきました。
- ページ内に表示する様々な要素の表示ロジックがController, Helper, View, Model内に散在しており、かつテストコードが十分に書けていない
- 既存実装の理解に手間がかかる
- 新規実装のときに「この処理はどこに書くのが適切か？」で迷う
- 新規実装の動作を自動化テストで保証しづらい
- 検索ロジックそのものの全体像が分かりづらい状態になっており、結果として同じような処理が何箇所化に重複して実装されている
- 本当は一つで済むはずのものが3箇所に実装されている上、挙動が微妙に違う
- 処理全体の依存関係が分かりづらいことでキャッシュの活用などパフォーマンスチューニングが実施しづらい
1つ目に関しては、いわゆるFat Controllerの問題と、ロジックが様々なレイヤー・クラスに散在している問題が絡み合ったものです。
「ページ内に表示する様々な要素」というのは、例えば次の赤枠のような箇所のことです。タイトルの「情報量が多いページ」はこうしたページ内に表示する要素が多いページのことを指しています。
赤枠の処理を具体的に書くと、
- 現在の検索条件と検索結果に応じたパンくずリスト／ページタイトルを表示する
- 現在の検索条件の選択内容を表示する
- 現在の検索条件に応じた検索結果を表示する
- 現在の検索条件と検索結果に関連するレコメンデーションを表示する
- 現在の検索条件に関連するほかの検索条件を表示する
- 現在のユーザの閲覧履歴に関連するレコメンデーションを表示する
といったものが挙げられます。
解決策の検討
よく言われる（と思う）プラクティス
どれも先輩社員にやんわりとアドバイスされたことがある内容ですが、改めてどんな理由でなぜ推奨されないのか？のリサーチを行いました。
今回は3つとも当てはまっています。
ビューに渡すインスタンス変数はなるべく一つに
DecoratorやPresenterを活用して、ビューに渡すインスタンス変数をなるべく1つにする
私にとってRailsの嬉しくない部分です。コントローラからビューにコンテキストを渡すのにインスタンス変数をいくつも使うのは、バッドプラクティスだと思います。Sandi Metzの言うとおり、インスタンス化して渡すオブジェクトは常に1つだけにすべきです
https://techracho.bpsinc.jp/hachi8833/2023_06_08/48322#09
Controllerにロジックを書かない
「Controller にビジネスロジックを書くな」と言われることがしばしばあると思います。
...
この状態だと
- Controller が肥大化し、コードの見通しが悪くなる
- Controller をまたがって共通化すべきロジックをうまく共通化しにくい
- 自動テストしにくい
などの問題が発生します。
https://qiita.com/os1ma/items/66fb47f229896b32b2e8
Viewにロジックを書かない
ビューには可能な限り機能を持たせないことが重要です。ビューのロジックが複雑になればなるほど、テストも困難になります。複雑になれば、HTMLやらボタンのクリックやCSSのセレクタなど、考えなければならないことがその分増えます。
この問題のシンプルな解決方法は、ロジックを別のクラスに移動することです。別クラスに切り出されたロジックならば単体テストも楽になります。単体テストは実行も早く、書くのも楽です。
https://techracho.bpsinc.jp/hachi8833/2023_03_10/55778
類似事例
部内の他プロダクトでの事例
昨年末から2ヶ月ちょっと部内の他プロダクトでの開発をお手伝いしていたのですが、そこでも同様の課題に遭遇していたそうです。
その解決策として、
-
app/responsesのように新たなディレクトリを切って - 各ページの表示内容を適切に返すことを責務とする
Responseクラスをページごとに定義し - Controllerが調達してきた
Responseクラスを頼ってViewを描画する
というような方式を取っていました。もちろん Responseクラス には適切なテストコードが書かれている状態です。
シンプルな方法で課題の大部分をクリアできているため、基本的にはこれと同じような方法を取れると良さそうだと思いました。
ただ Responseクラス という名前はほかでは聞いたことがないし、よくある設計パターンと比較するとどういう位置付けになるのかな…？という疑問が浮かび、それに関してもリサーチを行いました。
Sandi Metz' Rules For Developers
この話題に関連して色々と情報収集している中で、以下のポストを見つけました。まさに今解決したい課題に関して触れています。
原文をDeepLで翻訳したものが以下です。
コントローラ内でインスタンス化するオブジェクトは1つだけ
このルールは、実験を始める前に最も眉をひそめた。1つのページに複数の種類のものが必要なことがよくあった。例えば、ホームページにはアクティビティフィードと通知カウンターの両方が必要だった。
私たちはファサード・パターンを使ってこれを解決した。
https://thoughtbot.com/blog/sandi-metz-rules-for-developers#only-instantiate-one-object-in-the-controller
Presenterの活用
Viewで必要な複数モデルにまたがるデータの取得・整形などの処理をまとめる責務としては Presenter の活用も挙げられています。
Decorator による実装は、基本的に単一のモデルでの表示ロジックの集約というユースケースを想定しています。 したがって、複数モデルにまたがる表示ロジックがある場合や、たくさんのモデルをViewで扱う必要がある場合などは、 Decorator だけでは対処が難しいのです。
そんな場合は Presenter を使うのが効果的です。
Presenter は、 Decorator と比べるといまいち影の薄いパターンのように思うのですが、複雑なロジックを整理する上では非常に強力です。
https://tech.kitchhike.com/entry/2018/02/28/221159
Fat Controllerではインスタンス変数がどんどん増える問題もありますがそれも解消します。
Rails Wayに沿って愚直に開発していると場合よってはcontrollerにはインスタンス変数が増えていきます。また、set_xxxといったprivateメソッドをbefore_actionで呼びまくるみたいなことにもなりがちです。
...
自分たちのプロジェクトでもbefore_actionにset_しまくって明らかに可読性が下がっている箇所があったりしました。（Rubocopに従っているとなりがちです）
しかし、presenterであれば上記のインスタンス変数も1つになり、可読性の向上につながります。
https://qiita.com/d-haru/items/b1d4ee97b7dd5a98c697
Presenter, Modelってなんだろう？
色々書いてありますが、一言で言えば「PresenterやModelって呼ぶのはなんか違う気がするからPageクラスって呼ぶことにした」という話です
調べた結果、自分がほしいものは Presenter にかなり近いということが分かりました。以前に見かけた Responseクラス も同様です。
ここで一つ疑問だったのが、PresenterはViewとModelとを仲介する立場にありますが、例えば
- 現在の検索条件と検索結果に応じたパンくずリスト／ページタイトルを取得する
- 現在の検索条件の選択内容を取得する
- 現在の検索条件に応じた検索結果を取得する
- 現在の検索条件と検索結果に関連するレコメンデーションを取得する
- 現在の検索条件に関連するほかの検索条件を取得する
- 現在のユーザの閲覧履歴に関連するレコメンデーションを取得する
といった処理は、本当にただの「仲介」なのか？ということです。
これらは「検索結果ページ」というモノがユーザと事業にとって期待通りに動くため正しく実装されなければならないビジネスロジックですが、それってもはや 検索結果 というモノを表現するModelに実装されるべきものではないか？という疑問が浮かびます。
一方で「現在のユーザ」「現在のcookie,session」に応じた処理や、Viewのための表記の整形処理などを書くこともあるという点では、他のModelよりもプレゼンテーション寄りの事情を知っているため、本当にModelになのか？という疑問も浮かびます。
ということで、ここでほしいのは「各ページを構成する各要素を適切に調達してくれる責務・レイヤー」なのですが、現在のユーザ・検索条件・検索条件に応じた検索結果・cookie, sessionなど様々な要素に依存し既存の枠組みに収まりきらないことと、各ページに対応させる考え方との分かりやすさから、 Pageクラス という名前で呼ぶことにしました。
ちゃんと分ければ「この部分は厳密にはModel」「この部分は厳密にはHelperとController」のように分けることもできると思うのですが、チームでルールをスムーズに運用していく中での負担を考えると、「とりあえずそのページに関する処理を色々突っ込んでもいい場所」を定義するのがリーズナブルだと判断し、このような結論を選びました。
他の手段との比較
Serviceクラスの活用
ControllerにもModelにも置きづらい処理を押し付けていくという点では、Serviceクラスとも近いものがあります。
それでも実装はできるのですが、ふんわりと位置付けのあいまいなServiceクラスに置くよりは、今回はPageクラスという概念を新たに定義することでチームでの設計実装の方針を立てやすくなることを期待して、Serviceクラスとは分けてPageクラスというものを定義しました。
FormObjectの活用
保存処理で複数モデルにまたがる処理をFormObjectでまとめる話も聞くことは聞きますが、今回は読み出しがメインの処理なのでちょっと違うかな？ということで見送りました。
ViewComponentの活用
Viewに関連するロジックをViewComponentとしてまとめて定義するという手もあります。
今回は小さく始めたかったことと、ビジネスロジックも書く場所として適切なのか？が微妙なので見送りました。
改善策の概要
Pageクラス本体
実装イメージとしては、以下のように app/pages 内にPOROでクラスを定義して、このクラスに対してテストも書いていきます。
必ずしも Controller#action と対応するとは限らないのでクラス名の命名は柔軟につけていく想定です。
class SearchResultPage
# 検索条件、ログインユーザなど必要な引数を受け取る
def initialize(search_condition:)
@search_condition = search_condition
# ...
end
# 検索結果の施設リストを返す
def found_facilities
@found_facilities ||= ...
end
# meta要素のdescriptionの内容を返す
def meta_description
# ...
end
# titleタグに表示するページタイトルを返す
def page_title
# ...
end
# ...
end
class SearchResultController < ApplicationController
def index
@search_result_page = SearchResultPage.new(...)
end
end
<%= @search_result_page.page_title %>
共通処理・名前をつけて切り出したい処理を切り出したオブジェクト
複数のPageクラス内で共通して呼び出される処理や、Pageクラスの可読性・各種ロジックをカタログのように管理していくという点で、一連の処理を名前をつけて切り出したい場面が出てきました。
その場合は特異メソッドを一つだけ持つModuleを定義しても良いことにしました。
module BuildSearchMetaDescription
class << self
# メソッド名はすべてexecuteで統一する
def execute(...)
# ...
end
end
end
その他Pageクラスを実装するうえで定義しておくと便利なクラス
検索条件 など、クラスとして定義しておくと便利なものは（ハッシュではなく）クラスとして定義します。
これも app/pages ディレクトリに入れて良いことにしたのですが、 app/models に入れるのでもよかったかもしれません。
チームのメンバーとのディスカッション
この新たな設計実装方針に関して、開発チーム全体でも説明会とディスカッションの時間を取りました。
基本的には、前々から挙がっていた処理をどこに置くのか？テストをどうやって書くのか？の問題が解消するということで、肯定的に受け止めてもらえました。
「ServiceやPresenterに似てるね」といった声もありましたが、「似てるけどちょっと違う処理も入れられるように違う名前にしたのだ！」と説明して納得してもらいました。
関連する余談：YARDの導入
これまでコード内のメソッドの引数・戻り値・例外などのインタフェースを記述する方法をチーム内で統一していなかったのですが、これを機にYARDを導入しました。
選定理由としては、エディタの入力補完を受けやすそうということと、将来的にRBSを導入することを見越してクラスに関する情報をコメント内に埋め込んで起きたかったということがあります。
# メソッドがやることの簡単な説明
#
# @param 引数名 [クラス名] 引数の説明
# ...引数は個数分書く
#
# @return [クラス名] 戻り値の説明
def hoge_method
# ...
end
参考：テストピラミッド
単純なCRUDを実装するだけなら実装もテストも簡潔に書けますが、いろいろなものを表示しようとするとその分様々な処理がControllerに実装されていく傾向にあります。さらにいろいろなものを表示する分、表示パターンの組み合わせそれだけ増えていくため、必要なテストケースのパターンも爆発的に増えていきます。
つまり、プロダクトを構成する機能の複雑さに応じて、必要なテストの手法も変わってくることになります。
この議論を分かりやすくするために、単体テスト→統合テスト→E2Eテストの順に各階層が重なっている様子をピラミッドの形に見立てる考え方があります。
- ピラミッド型
- 単体テストが多め
- E2Eテストは実行に時間がかかるしパターンも多くなってしまうし失敗時の原因特定が大変なので単体テストを多めにするのが良い
- 逆ピラミッド型
- E2Eテストが多め
- 例えば単純なCRUDが多い場合はE2Eテスト中心でも必要なテストができる
- ダイヤモンド型
- 単体テスト・E2Eテストが少なく、統合テストが多め
今回のこの記事の話は、「逆ピラミッド型：機能がある程度複雑なのにも関わらず、ページ全体の単位でざっくりテストすることしかできない状態」だったものを、「ダイヤモンド型：機能を構成する各コンポーネント同士の連携を適切にテストできている状態」に寄せていくための工夫と言ってもよさそうです。
テストピラミッドについてはこちらの記事などを参照すると良いかもしれません。
（ポエム）レイヤー分けとか責務の分担とか自動化テストとかってなんでやるの？
こんなふうにあれこれ検討していく中で、そもそもなんでこの整理に時間使ってるんだっけ？も少し整理してみることにしました。
大前提として、自分たちはエンジニア組織に所属してプロダクトを開発しているわけですが、専門職集団であるそのエンジニア組織のアウトプットとして何を求めるか？という点を考えたときに、避けなければならないのは「動いているなら動きますね（なんで動く？なんで動かない？を自分たちが把握・理解・説明できない状態）」だと思います。
「エンジニア」の仕事を「仕組みを作る」ことだと捉えたときに、どうしてその仕組みが動くのか？を理解していないのはさすがに無責任だろう、という話です。
それならエンジニア組織以外の誰かが見様見真似やコピペやChatGPTで実装するのと何が違うんだという話になるわけで、エンジニア組織というものを作って分業する意味も薄れてきてしまいます。
ということで、自分たちが作っている仕組みをいかに自分たちの手中・管理下に収めていくか？という点が問題になってきます。
人間の認知能力には限界がある・人間が使える時間にも限界があるという制約を考えると、
- 今ここで意識しなくても良いものは忘れられる仕組み
- 抽象化によりインタフェースの使い方だけ分かっていれば欲しい結果が得られる状態
- 何が何に依存しているのか？がわかりやすい状態
- 信じられるはずのものが信じられる状態を維持する仕組み
- インタフェースが定義どおりに動くことを保証できる状態
- 何が何を依存していい・いけないのか？のルール（縛り）が明確に定義されている状態
といった工夫が必要になってきます。ということで、
- 切り出せる一連の処理は切り出して利用可能なインタフェースとして定義しておく
- そのインタフェースが信じられる状態を維持できるよう自動化テストを書く
- 処理のまとまりを「レイヤー」に分類して、レイヤー間での依存関係のルールを作る
- ディレクトリ分けやlinterなどでレイヤーによる分類をしやすくしておく
といった形で仕組みの作り方に「縛り」をつけることで、自分たちのアウトプットのクオリティを上げる工夫をしていると言って良いと思います。
リファクタリングの実施
before/afterの図示
今回のスコープのページはいくつかあるのですが、特に工数が大きかった検索結果ページの整理をご紹介します。
before
ページの構成要素の表示のために必要なビジネスロジックが、View, ViewHelper, Controller, ViewModelに分散していました。
さらに同じ 検索条件に応じた絞り込み処理 が3箇所に実装されていました。
ちなみに赤マーカーが3箇所ありますが、対応するのは以下の3箇所です。
リリース当初の設計にあった拡張性や柔軟性が、かえって分かりづらさ・扱いづらさにつながっていた部分もあったため、途中から重複を承知の上で新機能を実装する判断になったのは一定仕方のない部分もあったのだと思います。
今回は「とにかく扱いやすい実装にする」という考え方で作られた③の実装を活かす形でリファクタリングを進めました。
after
ページの構成要素の表示のために必要な処理のうち、既存のModelに定義するのが適当ではないと判断したものは、すべて pages ディレクトリ以下の Pageクラス と module に移動しました。さらに重複した実装も解消されました。
作業
次のような流れで移行作業をどんどん進めていきます。
- 下準備
- 移行前の処理について理解を深める
- 移行前の処理でテストコードが無い箇所はテストコードを書く
- バグや曖昧な仕様を発見した箇所はPdMに確認
- テストがそもそもなかったり、あるけど網羅できていないパターンがあったり、もありました
- （必要に応じて）移行対象処理の呼び出し元のテストを追加する
- 移行先準備
- 移行先に実装とテストコードを書き写す
- 移行元の処理の実装を移行先への呼び出しに切り替え
- 移行先の実装をすべて消して
移行先を呼び出す()だけに書き換えます - このときに移行元にあったテストが落ちないことが重要です
- 移行先の実装をすべて消して
- 呼び出し元での処理切り替え
- 移行元処理を呼び出すのをやめて、直接移行先の処理を呼び出すように変更します
- ゴミ掃除
- いらなくなった旧実装とテスト、呼び出し元の冗長なテストを消す
と手順を書くのは簡単なのですが、実際に稼働中の注力プロダクトに手を入れていくことになるので、かなり集中力の要る作業でした。
全体を通して、最終的には50以上のPRをリリースすることとなりました。次々とOpenされるPRをめげることなく（？）レビューしてくれた主に3人のメンバーには大感謝です。
検索処理を実質的に1から実装し直す大幅なリファクタリングでしたが、バグを出さずに作業を完了させることができました。
むしろ作業の中で既存実装のバグや仕様の考慮漏れを何箇所か見つけられたり、存在するけど認知されていないページを消せたりといった成果も上げることができました。
一部メンバーに作業を手伝ってもらったのですが、結果的に「リファクタリングの前にテストコードをちゃんと書こう！」「処理を移動するときはこういう手順で進めよう！」「こういう条件のときのテストコードはこうやって書くといいかも！」など、テストやリファクタリングに関して経験を積む機会にもつながったと思います。
最後に
この設計実装方針を始めてから半年以上が経ち、今となっては「このControllerって色々処理書いてあるけどテストコードが不十分だね」「じゃあPageクラスに移動してテストも書き足しておこうか」のような会話がチーム内で自然に生まれるようになりました。
設計実装の中でも、以前のように「これってControllerに書くの？Helperに書くの？」のような議論で迷うことは減ったように思います。
これにより、コードの保守性の改善であったり、自動化テストにより動作が保証された状態の維持であったりという点で、チームのアウトプットを質・量ともに底上げすることにつながっていると良いなと思います。
厳密なレイヤー分けという意味では妥協した部分もあったのですが、チームの各メンバーが無理なく実践できる方法としては、今になって思うとそれなりに良い選択だったんじゃないかと思います。
今後の課題としては、PageクラスとServiceクラスの役割分担が曖昧になってきたので、「どんな処理はどっちに書く」をもう少し解像度高く定義していく必要がありそうです。
次回予告
次回は @ancient_city さんが担当します。お楽しみに！
この記事の他に2つ記事を書いています。よかったらご覧ください！