トレタ開発者ブログ

飲食店向け予約/顧客台帳サービス「トレタ」、モバイルオーダー「トレタO/X」などを運営するトレタの開発メンバーによるブログです。

奥野さんと社員のリファクタリング部屋 -ディレクトリの名付け方

「奥野さんと社員のリファクタリング部屋」は、リファクタリングに励むトレタの社員と技術顧問の奥野さん ( @okunokentaro ) の間で実際に行われた会話を切り取った開発現場実録コンテンツです。

技術顧問: 奥野さん
三度の飯よりリファクタリングが好き
今回の質問者: 武市さん
トレタ在籍2年。沖縄在住のフロントエンジニア

今回の質問

前回に引き続き、Webアプリケーション (Next.js) のプロダクトのリファクタリングを進めている武市さんから、ディレクトリ構造のリファクタリングについての質問です。

tech.toreta.in

前回の指摘も踏まえて、新しいディレクトリ構造とその定義を考えてきました。


┗ server
 ┃ ┣ boundary -- 外部システムとのやり取りを行うためのエンドポイント。
 ┃ ┃ ┗ mojito
 ┃ ┃ ┃ ┗ mojitoClient.ts
 ┃ ┣ handlers -- リクエストやエラーのハンドリングを行う。具体的には、HTTPリクエストを受け取り、適切なユースケースを呼び出して処理結果を返す。
 ┃ ┃ ┗ item
 ┃ ┃ ┃ ┣ delete
 ┃ ┃ ┃ ┃ ┗ deleteHandler.ts
 ┃ ┃ ┃ ┣ get
 ┃ ┃ ┃ ┃ ┗ getHandler.ts
 ┃ ┃ ┃ ┗ put
 ┃ ┃ ┃ ┃ ┗ putHandler.ts
 ┃ ┣ models -- BFF固有のオブジェクトやエラーを定義する。
 ┃ ┃ ┗ conflictError.ts
 ┃ ┣ repositories -- データの永続化層とやり取りを行う。データベースや外部APIとの通信を担当。
 ┃ ┃ ┣ daleteItem
 ┃ ┃ ┃ ┣ adaptResult
 ┃ ┃ ┃ ┗ delete.ts
 ┃ ┃ ┣ getItem
 ┃ ┃ ┃ ┗ get.ts
 ┃ ┃ ┗ updateItem
 ┃ ┃ ┃ ┣ adaptResult
 ┃ ┃ ┃ ┗ update.ts
 ┃ ┗ useCases -- ドメインロジックを含む。ビジネスルールを実装し、リポジトリと連携してデータの取得、更新、削除を行う。
 ┃ ┃ ┗ item
 ┃ ┃ ┃ ┣ deletItem.ts
 ┃ ┃ ┃ ┣ getItem.ts
 ┃ ┃ ┃ ┗ updateItem.ts

ディレクトリを分ける目的を考える

奥野 まず、「ディレクトリを細かく作りたいのはなぜか」という本質に立ち替えってみましょう。例えば、srcっていうディレクトリに全てのファイルが全て兄弟で並んで、何百個というファイルがフラットに並んでいても別に構わないし、そういった構造をとることが可能か不可能かでいったら、可能ですよね。

でも、なぜそうはしたくないのか、なぜファイルを分けたくなるのか、というところを考えてみましょう。それもファイル名のアルファベット順で分けることだってできるのに、なぜboundaryに分けてhandlersで分けるのか、というところですね。

こういった分け方をするのであれば、ディレクトリを分けるルールを決めるだけではなく、そのルールに従い続けられる環境をどうやって構築して、その環境を維持するかも一緒に議論しないといけません。

つまり、ディレクトリを分けたら終わりではなくて、今後作るファイルをディレクトリ分けのルールに従わせ続けることが目的ですよね。ディレクトリの分け方を決めて満足してはダメで、1年後、2年後まで維持し続けられるのか、どうやってルールの適用を自動化するかを考えましょう。そう考えた時に、このディレクトリのルールをどうやって守り続けますか?

武市 これは奥野さんのプロジェクトを参考にさせていただいてますが、参照可能なディレクトリの範囲を決めて、それ以外を参照してた場合にはCIの時点で落とすっという処理が、僕も適切だと思っています。(編注:eslintなどのルールを組み合わせることを指しています。)

今回で言うと、例えばここではserverの配下にmodelsというものがありますが、このserver配下のmodels内のファイルはserverより上のディレクトリのファイルは参照してはいけない、というルールは簡単に導入できるなと考えていました。

奥野 そうですね。 その場合、依存のルールを厳しめに作り、依存の方向に制約をかけることでなにが嬉しいかというと、例えばテストを作る時の負担が減りますね。「このレイヤーにテストを書きたい」と考えた時に、「どのレイヤーはモックにしよう」という方針をたてやすくなる。

守られていなかったら、ファイルごとにものすごく激しい密結合を生んでいるファイルもあれば、ちゃんと疎結合になってるファイルもあればで、ファイルの実装を一個一個読んでいかないとテストの方針が立てられない。依存のルールが決まっていることによって、このディレクトリに入っているファイルだったら依存のルールが統一されているお陰で、一つのテスト方針でディレクトリ内全てのファイルに対してテストが同じように繰り返し作成できる、という流れに繋げられる。

モックをどういう粒度で作っていくか考える時に、依存の粒度が揃っているのはすごくテストを書く上で扱いやすくなりますね。

それって本当にリポジトリ?

奥野 こういうディレクトリに分けをするときは、「分けて満足してはダメで、分けた後にそれを維持し続ける仕組みをどうやってメンテナンスするか」がとても重要になってくる。

そう考えると、今回の分け方で妥当なのかっていうと、boundary以外のhandlers models repositories useCasesはちょっと怪しいなって思っています。特にrepositoriesで分ける必要に本当にあるのか、という部分。

useCasesboundaryが分かれていればそれで十分なのではないかという考え方も自分は持っていて。repositoriesで分けたい理由が明確になってないうちにrepositoriesというディレクトリを置くルールに決定してしまうと、そこを中心にテストでモックを準備したり、なにかのファイルを作成するときに要らぬ手数が増えたりすることになるのではないかという懸念があります。 useCasesboundaryで十分なところに、わざわざリポジトリを設けることで迂回して遠回りすることになりかねないから、そのrepositoriesで分けたい理由はちゃんと聞いておきたいです。

武市 はい、useCasesはバックエンドから返ってくるものを扱うので、モックするのが正直面倒だと思っています。useCasesはビジネスロジックを中心に扱いたいので、バックエンドからどんな値が返ってくるかまでをモックしたくないな、と考えました。

奥野 なるほど。でもuseCasesのテストでバックエンドをモックをしたいなら、この場合boundaryをモックすれば十分とも見られます。なのでuseCasesboundaryの間にrepositoriesを置きたいという価値はちゃんと議論しましょう。

武市 useCasesでやることの中には、repositoriesgetItemとかdeleteItemを両方呼ぶ可能性もあります。

例えば、updateItemのユースケースの場合、名前がすでに使われていないかデータベースに一回取得しにいくので、updateItemのユースケースの中ではリポジトリのgetItemも呼ぶ必要があります。

そのような場合にリポジトリを分けておけば、汎用性高く使いまわすことができるというのがuseCasesrepositoriesを分けるメリットかなと思ってます。

奥野 それはリポジトリという名前で呼んではいるけど、Repository Patternの踏襲にはなっていなくて、あくまでもuseCasesの中の共有処理に過ぎないのでは、と感じました。 全体のユースケース = シナリオのように扱える部分があって、粒度の細かいユースケースを組み合わせて一つの大きなシナリオを組み立てているという感じですね。なので今のディレクトリ構成案では、repositoriesが最小のユースケースであり、useCasesと今呼んでいるものはもっと大きいシナリオのような粒度ではないかと思いました。

ということは「ユースケース」という言葉に対する認識が自分と武市さんの間で揃っていないし、「ユースケース」という言葉の粒度を定義しないとチームの他のメンバーにも伝わりにくくなってしまいます。

意味が伝わりやすい名前にする

奥野 たとえば、Clean Architecture *1の本に書いてあったとか、達人プログラマー *2に書いてあったとかであれば、「あの本でいうユースケースです」みたいに説明すればいいと思うんですが、色々なところからつまみぐいを繰り返している結果、全体的によくわからないアーキテクチャになってしまっている気配があります。

リポジトリというものはDDD(ドメイン駆動設計)*3の言葉ですよね?Clean ArchitectureとDDDを両立することもできるけど、分かって組み合わせないと、かいつまんだだけの全体的に何がしたいか伝わりづらいアーキテクチャになってしまう。今回の場合、このアプリケーションではDDDを実践していないので、そこで無理にrepositoriesという言葉を使う必要もないと思っています。

あくまでも複数のユースケースを連ねるものだから、ユーススケースの粒度を細かくしたものにすぎなくて、ドメイン駆動設計としてのリポジトリではない。言葉だけ拝借してるけどルールがちゃんと定まってないから、「ユースケース」や「リポジトリ」という言葉では武市さんの説明を聞いても自分はあんまりピンときてなかったんですよ。

これは、武市さんのやりたいこと、やろうとしてることを否定したいのではなく、やろうとしてることはわかるんだけど、語彙からそれが伝わってこない、っていうところがポイントですね。

例えば、さっきのupdateItemというユースケースを例にすると、getItemで商品情報を取得し、取得した後にupdateItemを行う…というようにデータベースの操作と密にもなりつつ、ドメインロジック・ビジネスロジックとしてもこう取り扱っておきたい、みたいな粒度がありますよね。

そんな複数の処理のコンビネーションをなんと呼ぶべきか、そしてその一個一個をなんと呼ぶべきかというところを整理していくと、もうちょっと納得感のある切り方になると思います。自分は、ドメイン駆動設計を実践しておらずモノリシックな処理をとにかく細分化したいという今の段階において、ここに凝った名前をつける意味はあんまりないと思っていて、自分が担当しているプロダクトではhandlers-sharedと名付けています。そこではHTTPハンドラがいっぱいあって、そのハンドラが一つ一つ細かい操作を決めるんだけど、複数のハンドラで共有したい概念があったらそれはhandlers-sharedに置いてます。

自分の場合はhandlersの中で共有したいものなのでhandlers-sharedに置くけど、武市さんの例の場合だったらuseCases/sharedとかにするとよいかと思います。useCasesの粒度であることに変わりはないけど、useCasesの中で単独で使われるか、共有で使われるかというところの違いでしかありません。

useCasesuseCases/sharedで分けるのがしっくりこなければ、極端な話、全部useCasesに突っ込んでuseCasesboundaryだけでいいんじゃないかというのが自分の最初の主張なんですよ。

最初にboundary以外全部しっくりこないって言い方をしましたが、boundaryuseCasesmodelsの3つでも十分回ると思ってます。大切なのは名前が何かではなく、ファイル間の依存関係がスパゲティのように絡まないかということです。

ルールを納得させる

武市 確かにそうですね。自分の考えとしては、ユースケースはデータベースのことを知らないようにしたくて、ディレクトリを分けた理由としてリポジトリはDBのことを隠す役割ために分けたいからと考えていました。

奥野 どのシステムに繋ぐかを隠してその操作の抽象を取り扱ってるという考え方は合っていると思うけど、武市さんの感覚でリポジトリと名付けて「でも世でいうリポジトリとは異なります」だと、今後チームに参入する人に納得してもらうには武市さんの話術にかかってくると思うんですよ。世でいうリポジトリとは異なるのはなぜなのか?を毎回説明することになる。それだったら説明しづらい言葉を入れるよりも、もうちょっとフラットに通じる名前にしたらいいんじゃないかっていうのが自分の考え方ですね。

ちなみに自分のとこではside-effect(副作用)って呼んでます。

外部に対して何らかの変更をもたらす、追加したり消したりとか、そうゆうハンドラ関数における副作用ですね。なのでそのハンドラが本来何をしたいのかっというside-effectというものがいくつも並ぶことによって何らかの目的を達成させます。武市さんの言葉を借りるとユースケースですね。

自分の前に携わっていた案件だと一つのハンドラー内はincomingside-effectoutgoingの3つに分けていました。

┗ handlers
 ┃ ┣ users
 ┃ ┃ ┣ get
 ┃ ┃ ┃ ┣ incoming
 ┃ ┃ ┃ ┣ side-effect
 ┃ ┃ ┃ ┗ outgoing

全てのハンドラに対して一様に必ずこの3つに分けていて、リクエストっていうのは必ずバリデーションをしないといけないし、認証認可の文脈でも何らかの処理が必ず発生する。エンドポイントというのは入ってきたものを検閲するポイントが必ず必要になってくる、それがincomingになってる。

incomingで合格した値っていうのは、そこから何か処理を呼んだり、書き込んだ消したり、必ず何らかの副作用を起こします。他のシステムに通信を飛ばしたり、そういうハンドラの副作用となる部分を全部side-effectに入れてます。

さらにそのside-effectが終わった後の結果っていうのは何らか返したいっていう欲求が絶対ある。それはHTTPのハンドラを実装してる以上、何かしらのHTTPレスポンスを返したいっていうのは当然の振る舞いなので、そこをoutgoingディレクトリ内の処理で、レスポンスのバリデーションをしたりとかレスポンスのBodyの構築をしたりとかをする。だから、「入口」・「やること」・「出口」で分けている。

武市 なるほど。

奥野 自分が前に携わっていた案件の言葉を使って、武市さんのこのディレクトを説明すると、今はuseCasesincomingside-effectoutgoingが全て詰まっていますね。そのside-effectの中でもうちょっと分けておきたいなっていう単独化というか抽象化を、武市さんの言葉でいうリポジトリでやってたわけですよ。

このディレクトリに名付ける名前は正直なんでもよくて、みんなが納得してみんなで守れたらそれでいい。武市さんが「リポジトリ」って言葉を使うことを自分は何も禁止しないし、まったく否定する権利もないけど、ちょっとだけ忠告すると「リポジトリ」って言葉にはすでに色が付きすぎているから、「リポジトリ」じゃないものに「リポジトリ」って付けたときに他人に抱かせる抵抗感はそれだけ大きくなるよ、っていうものです。

武市 そうですね、本来の目的からけっこう外れちゃってますね。

奥野 「リポジトリ」と名付けたらRepository Patternを想起させるものでないと、人によっては「リポジトリちゃうやんけ」となる。

これはちょうど「リファクタリングとともに生きるラジオ」のデザインパターンの回 *4でも言ったんだけど、Strategy PatternとかSingleton Patternと、名前を付けるだけですぐ何をしたいのか世界中の人が共通で分かるっていうところがメリットで、Repositoryもそれなりに市民権を得ている。だからそういう言葉ほどそれに忠実であった方が求められる。

バウンダリ(外部システムとの連携部分)を隠蔽したいってのは分かるけど、それはバウンダリを隠蔽してるだけでリポジトリを模してるとは思えなかった。そうでないものに「リポジトリ」って名付けたときに他者を納得させるにはどうすればいいか、ってところで頭を抱えることになると思います。だから自分のとこではRepository Patternを踏襲してはいないため、リポジトリって言葉を意図的に避けていて、あえて使ってないんです。

でも結局やりたいことは、武市さんのここに書いてることと自分のとこも一緒です。それは外部と内部の抽象化をしたいのと、全体的な一連の結合テストにその個別のブラックボックスを取り扱いたくないっていう部分。だからやりたいことは一緒で、あとはそれをどう見せるかと、どうチームを納得させるか。

リードエンジニアに求められるポジションというか素養って、ルールを作る力じゃなくて、作ったルールを納得させる力なんですよ。ルールって作っても従ってもらえなければ破綻するので、ルールを作るだけじゃなくて、これに従ってねっと言って、「OK、従います」って周りのチームメイトに納得させる力なんです。そうなった時にリポジトリっていう言葉を使ってまで危険な橋を渡るのかっていうことです。

武市 めちゃめちゃ納得です!!

奥野 だからこのディレクトリツリーと、そこからくるやりたいことの説明はなにひとつ間違ってないんだけど、勇気がいる行いだと思いました。そこをもうちょっと冷静に考えてみたときに、リポジトリじゃないものをリポジトリと呼ぶのは説得力に欠けると立ち返るのであれば、他の言葉を選んだ方が無難ですね。

武市 そうですね。これから入ってくる人がわかりやすくなるようにディレクトリで役割を持たせよう、というのを目的にしていたのに裏面に出ていました。ありがとうございます!

To Be Continued…

PR

奥野さんがパーソナリティを務める「リファクタリングとともに生きるラジオ」も配信中です。リファクタリングに興味のある方はぜひチェックしてください!

refactoradio.com

リファクタリングの道標:リファクタリングを通じて組織を改善していく

こんにちは。トレタ技術顧問の奥野 (@okunokentaro) です。先日のブログ記事『奥野さんと社員のリファクタリング部屋 -リポジトリ層のディレクトリをどう作る?-』に対して多くの反響をいただき、ありがとうございます。いただいたご意見やご質問の一つひとつが非常に貴重であり、大変感謝しています。

いくつか個別に質問をいただくことがあったのですが、それらの質問を要約すると「リファクタリング後のコードもまだツッコミどころだらけでは?」や「リポジトリ層という名前がまずおかしいのでは?」という二つの点に集約されました。これらの質問はとても理解できます。今回の記事では、具体的な個別の質問への個別の回答は控えさせていただきますが、全体的な方針とテーマについてお伝えした上で、トレタが取り組んでいるリファクタリングは何を目指しているのかというお話をします。

前提と目的

今回のブログ記事『リファクタリング部屋』で取り上げているコードは、実運用中のアプリケーションの実際のコードで、すでに顧客が存在するプロダクトです。このアプリケーションは複数人で開発されましたが、現在は抜けたメンバーもいるため、開発当時の状況が分かりづらい箇所もあります。

また、本アプリケーションの開発中には筆者は別プロダクトのリードエンジニアを務めていたため、本プロジェクトへの参入からまだ日が浅く、山積している課題を目の当たりにしている状況です。すでに運用中であるため、すべての変更は何よりもエンバグを防ぐことが最優先とされ、ただでさえ自動テストが少ない状況で、何をリファクタリングするにしても慎重な検証が求められます。

このような状況が続くと、リファクタリングのひとつひとつにも労力が掛かり、難読なコードは新しく参加するスタッフに対しても良い印象を与えません。機能追加が進まないだけでなく、現状のバグ修正すら困難となったまま新しいスタッフを迎え入れるのは非常に忍びないです。

そのため、少しでも現存のスタッフ、そして今後参加する未知のスタッフのために環境を整えていくことが、今回のリファクタリングの目的です。

積み重ねを重視する

今回のリファクタリングは、増改築のための基礎工事というよりは、ひたすら庭の雑草むしりに近いものです。限られた時間の中で、テストも少なく大胆なリファクタリングを実施したときのエンバグのリスクが大きいことも考慮すると、なるべく確実と思われる手法で、なおかつ短工数で大きな効果を生みたいところです。

TypeScriptでの開発を進めていることから、幸い変数名や関数名の修正、ディレクトリの移動やファイル名の修正などは低リスクで実施できます。短すぎる、あるいは長すぎる名前が多く、主旨が判然としない処理も、名前を見直すだけで見通しがよくなることがあります。

また、テストがあまり作成されていないためテストを増やしていきたい気持ちも強いのですが、依存関係の管理がなされていないことから、モックテストを気軽に追加することすら困難です。

このような手に負えない状況から、低リスク短工数で実施できて効果の大きいリファクタリングを考えたときに、残されたスタッフが読み解けないコードを少しでも読解できるようにする必要があると判断しました。どんな改善も読解と観察から始まりますが、まずその読解が困難であるためです。

よって、TypeScriptのコンパイルエラーが出ないように改名や切り出しを繰り返し、ブラウザ上で動作検証を行うという、地味な流れではありますが長期戦を覚悟した確実な積み重ねを重視します。

なぜリファクタリング後のコードもツッコミどころだらけなのか?

リファクタリングをテーマとした記事を掲載した上でこのような質問をいただくのは申し訳ないところではありますが、いくつか背景を説明したいと思います。

先の節でもお話しした通り、このシリーズでは地味な改善を積み重ねていくことを重視しています。そして、1日のリファクタリング作業に割ける時間も限られており、本アプリケーションの他の機能開発や、他プロジェクトへのフォローなど複数の作業を並行している中での地道な作業となります。記事のベースとなっている武市さんとの1on1リファクタリングタイムも、無制限に1日中実施できるわけではありません。

そのため、「記事にして皆さんに読んでいただくこと」を目的に、ある特定のコードを極端に磨き上げるようなことはしていません。むしろ、泥臭い現場の中途半端さを包み隠さずに届ける気概でいます。今回の例では、リファクタリング後のコードもまだ名前が怪しかったり、そもそもサンプルに登場するオブジェクトのプロパティ構成が理解しにくいままだったりと、記事で触れていない部分のコード断片も多くの「ツッコミどころ」に溢れていたと思います。それは承知しています。

今回の記事を通じて、まずはファイルの細分化を目指し、Repositoryとは何かという既成概念に囚われずに改善に取り組むことを優先しました。その上でJavaScriptの第一級関数を取り扱える特性を活かし、ファイルや処理を細分化することによって、名前を付ける対象を減らし、より可読性と情報量に優れた名前を付け直せるようにすることがテーマでした。

リファクタリングには正解も不正解も存在しないと思っています。その行いが時を経てどう改善につながるか、あるいは逆に悪い方向に向かってしまうか、それだけのことです。今回は積み重ねを重視する上で、悪い方向に滑っていかないようにフォローすることを重視しました。読者によってはリファクタリング後のサンプルコードが「不正解」に映ったかもしれませんが、トレタの現場ではこれを「前進した」と捉えています。

また、一つのコード断片を極限まで磨き上げてしまうと、今回ご紹介していない他の多くのツッコミどころだらけなコードが、すべて取り残されることになってしまいます。そうではなく、アプリケーション内のすべてのコードに対して満遍なく実施しやすく、かつ負担も少ないという「効くリファクタリング」を考えるようにしています。それがファイル分割の指針であったり、関数分割の指針であったりします。お手本のコードを「少しよくする」ことで、他のコードもお手本通りに「少しよくする」ことができ、全体が底上げできたらまた新たなお手本を一つ選び「少しよくする」という、地道な底上げをひたすら繰り返していくことを目指しています。

そもそもリポジトリになっていないのではないか?

ブログについてもうひとつよくいただいた質問が、「リポジトリ層」という表現や、コード中に登場するRepositoryについての取り扱いに対する疑問でした。この質問もとてもよく理解できます。

現状のコードについてリファクタリング案を出す時、他にスタッフがおらず素案もない場合は筆者が手順をすべて考えて実施していますが、今回はトレタ在籍2年の武市さんが主導でした。そのため、リファクタリングの全体的な方針や理想像はまず武市さんが考え、それに基づいて筆者が壁打ちに付き合うという関係性になっています。

ここで技術顧問という立場として、筆者は提案者である武市さんの案を真っ向から否定することを避けています。つまり「リポジトリ層を設けたい」という相談について、リポジトリ層とは呼べないから提案を却下するという流れには意図的にしていません。そして「その提案はリポジトリ層とは呼べない」といった批評もまずはこらえます。

そうではなく、武市さんの現在の経験と知識からリポジトリ層を設けたいと考えたこと自体を尊重し、その判断に至った経緯をヒアリングしました。

それによると、今回はテストがとても少ない上にGraphQLのライブラリとの密結合がひどく、テストを書くこともままならない点を改善したい。そのためにディレクトリを新規に作り、そこに細分化したファイルを入れたい、とのことでした。ここで、そのディレクトリにRepositoryと名付けることが最善なのかという議論はあえて行わず、まずディレクトリを分けるという判断を最大限に尊重しました。その上で、ブログ内ではcreateServiceRepository()という関数で巨大なオブジェクトを生成する是非についてのみ触れ、別の細分化を提案しています。

トレタの目指すリファクタリングの好循環とは

武市さんの経験量、知識量と筆者のそれには差がありますが、筆者の知識をもって提案を否定することは簡単です。リポジトリ層を設けるという提案自体を否定して、筆者がすべてのレールを敷くことだって可能です。しかし、今回本当に改善したいのはアプリケーションコード自体ではなく、そのアプリケーションコードの改善を通じて円滑になる組織そのものなのです。

冒頭でも説明した通り、新しいスタッフが参入しやすい環境作りを目的としており、コードの改善はあくまでも組織改善の手段のひとつとして捉えています。コードの改善は通過点であり、目的ではないのです。

そのためには、コードの改善の最大化よりも、長期的な組織の関係性を良好にしていくことが重要と判断しました。そのため、武市さんの提案に対しては否定ではなく、更なる知識や事例の共有によってフォローすることに努めました。リポジトリ層という表現がたとえ最適でなくとも、まずはその案をもとに短工数かつ高利益となる改善を探していく。それが今回のファイル分割や改名の第一歩であり、この姿勢がトレタの目指すリファクタリングの好循環を生むための姿勢です。

語彙の選択は難しいところです。色の濃い語彙ほど、組織を超えた共通認識がすでに世界中で形成されているものですから、あらゆる書籍の中から語彙だけを掻い摘んで採用することが不適切になる場面も存在します。筆者としては、多くのアーキテクチャ解説書籍について読解するときに「真似するために読むのではなく、よそさまの事情と思って読むとよい」と伝えています。そして「よそはああやっている。うちはどうやろう?」を考えるための足掛かりになればよく、書籍を語彙リストとして使ってはならないと説明しました。

正直なところ、「奥野さんがリポジトリ層という名前をスルーして他の部分についてだけ指摘しているのは珍しい」という意見もいただいております。「リポジトリ層」という語彙を記事のタイトルに含めることで読者から指摘が生まれるだろうことも承知していました。筆者の性格をよくご存知だなと思いながらも、今回の記事はあくまでも現場の複数日のやり取りの中の1コマであることをご了承いただけると幸いです。

改善はまだ始まったばかり

『奥野さんと社員のリファクタリング部屋』はシリーズを想定しており、まだまだ続いていく予定です。自転車の補助輪が外れていく姿を眺めるように、焦らずに見守っていただけると嬉しいです。今回はたくさんのフィードバックを頂きありがとうございました。また次回もお楽しみに。

奥野さんと社員のリファクタリング部屋 -リポジトリ層のディレクトリをどう作る?-

「奥野さんと社員のリファクタリング部屋」は、リファクタリングに励むトレタの社員と技術顧問の奥野さん ( @okunokentaro ) の間で実際に行われた会話を切り取った開発現場実録コンテンツです。

技術顧問: 奥野さん
三度の飯よりリファクタリングが好き。
今回の質問者: 武市さん
トレタ在籍2年。沖縄在住のフロントエンジニア

今回の質問

今回は初期リリースを終えたWebアプリケーション(Next.js)のプロダクトを担当している武市さんから、複数人で開発を進めてきて統率が取れなくなったディレクトリ構造のリファクタリングについての質問です。

APIで外部とやり取りしている部分をリファクタリングして、クリーンアーキテクチャに沿ってリポジトリを作ろうと考えています。

その中で、GraphQL APIレスポンスの結果を変換するアダプター関数(adaptGetIServiceItemsAggregateResult)はどこのディレクトリに置くのがいいですか?

下記はGraphQLリクエストを行うリポジトリの関数です。

src/server/boundary/repository/service/serviceRepository.ts

export const createServiceRepository = (
  gqlClient: GqlClientType,
  client: HasuraClient,
): ServiceRepository => {
  return {
    updateServiceWithExternalKeys: async (
      id: string,
      updateData: UpdateData,
      externalKeys: Record<string, string>,
    ): Promise<UpdateServiceWithExternalKeysResult> => {
      const mutationDocument = makeServiceMutationDocument(externalKeys);
      const result = await client.request<
        UpdateServiceResult,
        UpdateServiceVariables
      >(mutationDocument, {
        id,
        name: {
          ja_JP: updateData.name,
        },
        displayName: {
          ja_JP: updateData.displayName,
        },
        printName: {
          ja_JP: updateData.printName,
        },
      });
      return adaptUpdateServiceWithExternalKeys(result);
    },
    getIServiceItemsAggregate: async (
      itemManagementGroupId,
      name,
    ): Promise<GetIServiceItemsAggregateResult> => {
      const result = await gqlClient.getIServiceItemsAggregate({
        itemManagementGroupId,
        name: {
          ja_JP: name,
        },
      });
      return adaptGetIServiceItemsAggregateResult(result);
    },
  };
};

「単一責任の原則」から外れていないか

奥野

createServiceRepository()という関数の返り値がupdateServiceWithExternalKeys()getIServiceItemsAggregate() という関数を持つオブジェクトになっているのはどういった意図ですか?

武市

サービスリポジトリの中でサービスに関わることをすべてやってしまいたいと思ったからです。

奥野

それはgetIServiceItemsAggregate() という単独の関数でもよさそうにみえました。わざわざcreateServiceRepository() という関数でオブジェクトを返さなくてもいいかもしれない。

見たところ、引数のgqlClienthasuraClient は異なるユースケースのためのクライアントのようです。この2引数を両方必要とする関数がありません。つまりこのリポジトリは「単一責任の原則」から外れていて、テストする時にどちらか必要のないモックも作る必要があるんですよ。

つまり、getIServiceItemsAggregate() のテストをしたいだけなのにupdateServiceWithExternalKeys() の準備もしないとテストが始められない状況ってことです。

サービスリポジトリはオブジェクト定数ではなく、あくまでもディレクトリとしてrepository/service/getIServiceItemsAggregate.ts を作るといいと思います。

export const getIServiceItemsAggregate = async (
  gqlClient,
  itemManagementGroupId,
  name,
): Promise<GetIServiceItemsAggregateResult> => {
  const result = await gqlClient.getIServiceItemsAggregate({
    itemManagementGroupId,
    name: {
      ja_JP: name,
    },
  });
  return adaptGetIServiceItemsAggregateResult(result);
};

武市

なるほど。

奥野

今って処理をなるべく細かくしたいという段階ですね。細かくすればするほど一つのテストがしやすくなるためそうしようというモチベーションなのに、これでは小さくしたものをくっつけてまた大きくしちゃってます。

武市

たしかに。リポジトリというものに全部詰め込んだ方が、と思ったんですが。

奥野

リポジトリというのはあくまでも考え方で、外部のシステムに依存するものがリポジトリという場所にまとまっているといいよね、という考え方です。

まず、リポジトリ層というものがどういう経緯で生まれたかという歴史的背景をわかっていた方がいいと思います。

リポジトリ層はもともとJavaなどで取り入れられたクラスベースの考え方として広まっています。Javaはクラスしか扱えないからこそ、リポジトリクラスにインスタンスメソッドを生やす、必然的にコンストラクタに初期値を渡す、すなわちそこで依存性を備えるということをせざるを得なかったんです。

我々はTypeScriptプログラミングをしているので、必ずしもJavaの作法を真似する必要はなく1関数1ファイルというTypeScriptなりの書き方をすることもできます。

なので、repository/service/ というリポジトリのディレクトリの中にそれぞれの関数ファイル を作るという方法もありますよ。

repository/
    ├ service/
        ├ updateServiceWithExternalKeys.ts
        ├ getIServiceItemsAggregate.ts

細かく分けたいなら、今はとにかく細かく分けることに注力するといいと思います。


1責務 1 関数1ファイル

奥野

ファイルを分割することで、アダプタをどこ置くかという問題は自然に求まります。

関数を細かく分ければ分けるほどアダプターはそこでしか使わなくなる。なのでディレクトリをさらに細かくしていって、そこにアダプター関数を置けばいいということになります。

repository/
    ├ service/
        ├ updateServiceWithExternalKeys/
            ├ updateServiceWithExternalKeys.ts
            ├ adaptUpdateServiceWithExternalKeys.ts
        ├ getIServiceItemsAggregate/
            ├ getIServiceItemsAggregate.ts
            ├ adaptGetIServiceItemsAggregateResult.ts

これでなにが嬉しいかというと、adapt関数にそんな長い名前を付ける必要があるかと、いう問題の解決となる。

これまでなぜ名前を長くしないといけなかったかというと、今まで同じファイルに同居しており区別するための名前を付ける必要があったから。同じファイルにいないのであればadaptResult()で十分ですね。

もうちょっと言うと updateServiceWithExternalKeys.tsっていうのもupdate.tsで良いし、getIServiceItemsAggregate.tsgetAggregate.tsで十分です。IServiceItemsという名前はそれ自体がちょっと怪しいですね…。

repository/
    ├ service/
        ├ updateServiceWithExternalKeys/
            ├ update.ts
            ├ adaptResult.ts
        ├ getIServiceItemsAggregate/
            ├ getAggregate.ts
            ├ adaptResult.ts

もし武市さんが名前つけていて「この名前ダサいな」と思ったってことは何か良くないこと、微妙なことやってる証拠なんですよ。名前がおかしいってことは、そこで扱っている粒度がおかしいっていうことですね。

武市

疑問を持つタイミングが違ってました。リポジトリを作る際にgraphqlClinethasuraClientを注入しないといけない段階でキモいなと思ってたんですよ。

奥野

「キモい」という感覚はすごくいい一歩で、やっぱりキモいものって引数構成とか戻り値構成でどこか間違えてるんです。

オブジェクト指向を踏襲するとか、Dependency Injectionの考え方を踏襲するっていうのはすごくいい考え方ではあるんだけど、それは全てSOLID原則のS(単一責務の原則)を守ってこそ。 そういう各原則とかを守れてないあやふやなオブジェクトの状態であれこれ考えても、元が整っていないとツラいですね。 1責務 1 関数1ファイルという粒度にどんどん削ぎ落としていく。そうあるべき姿に持っていけば、リポジトリという巨大なオブジェクトは作る必要がなくなります。

武市

なるほど。先ほどキモいという言葉使ったんですが「違和感」 というものもうちょっと大事にしていこうと思います。


依存性注入とカリー化

奥野

getIServiceItemsAggregate()という関数を単独で切り出すと、引数がgqlClient itemMangementGroupId name の3つになりますが、もとのcreateServiceRepository()gqlClientの部分をわけていたのは、パラメーターではなくDependency Injection をやろうとしたということですよね。そこを分離させることは 賛成で、gqlClientもパラメーターっぽく見えるのは嫌ですよね。そこで使うのがカリー化です。

武市

依存だけ先に引数で渡すという話ですか?

奥野

その通り。TypeScriptで依存性注入する時はclass constructorでやるかカリー化した関数の引数でやるかが多くて、それをうまくできるような関数設計に寄せていくと綺麗にまとまっていきます。

type GetIServiceItemsAggregateResult = {}; // ダミー

type Return = (
  itemManagementGroupId: string,
  name: string,
) => Promise<GetIServiceItemsAggregateResult>;

export const getAggregate = (gqlClient: GqlClientType): Return => {
  return async (itemManagementGroupId, name) => {
    const result = await gqlClient.getIServiceItemsAggregate({
      itemManagementGroupId,
      name: {
        ja_JP: name,
      },
    });
    return adaptResult(result);
  };
};
type UpdateServiceWithExternalKeysResult = string;

type Return = (
  id: string,
  updateData: UpdateDate,
  externalKeys: Record<string, string>,
) => Promise<UpdateServiceWithExternalKeysResult>;

export const update = (client: HasuraClient): Return => {
  return async (id, updateData, externalKeys) => {
    const mutationDocument = makeServiceMutationDocument(externalKeys);
    const result = await client.request<
      UpdateServiceResult,
      UpdateServiceVariables
    >(mutationDocument, {
      id,
      name: {
        ja_JP: updateData.name,
      },
      displayName: {
        ja_JP: updateData.displayName,
      },
      printName: {
        ja_JP: updateData.printName,
      },
    });
    return adaptUpdateServiceWithExternalKeys(result);
  };
};

名前を短くして見通しやすくする

奥野

関数名を短くしたら全体的に名前を短くする工夫もするといいですね。

makeServierMutationDocument()という名前も、もしこの関数だけでしか使わないのだったら makeDocument()にしたらいいし、idstring型ではなくItemManagementGroupId 型ってわかれば別に変数名として長々と付ける必要はないから idで十分とか、読みやすくできる部分はどんどん読みやすくしていくと、さっきより読みやすくなった。

type GetIServiceItemsAggregateResult = {};
type Return = (
  id: ItemManagementGroupId,
  name: string,
) => Promise<GetIServiceItemsAggregateResult>;

export const getAggregate = (gqlClient: GqlClientType): Return => {
  return async (id, name) => {
    const result = await gqlClient.getIServiceItemsAggregate({
      itemManagementGroupId: id,
      name: { ja_JP: name },
    });
    return adaptResult(result);
  };
};
type ServiceId = string;
type Return = (
  id: ItemManagementGroupId,
  updateData: UpdateDate,
  externalKeys: Record<string, string>,
) => Promise<ServiceId>;

const update = async (client: HasuraClient): Return => {
  return async (id, updateData, externalKeys) => {
    const doc = makeDocument(externalKeys);
    const variables = {
      id,
      name: { ja_JP: updateData.name },
      displayName: { ja_JP: updateData.displayName },
      printName: { ja_JP: updateData.printName },
    } satisfies UpdateServiceVariables;
    const result = await client.request<
      UpdateServiceResult,
      UpdateServiceVariables
    >(doc, variables);
    return adaptResult(result);
  };
};

奥野

長い名前ってことは 長く説明しないと伝わらない概念ってことであって、それは概念整理できてないってことなんですよ。概念整理ができると、開発者同士で前提の共有ができるため短い名前でも通じるんですよね。

なのでなぜupdate()の一言で済むかというと、パスがrepository/service/updateだからなんですよ。/serviceの中にいるアップデート関数 だからサービスのアップデートなんだってわかる。

それはコンテキストが補えてるんですよね。

でもそれがコンテキストがないとupdateServiceWithExternalKeys()って全部の要素を名前の上で言わないと伝わらないし、名前以外の情報が足りてないってことだとわかります。だから repository/serviceいうディレクトリを切ったおかげでそれが何かっていうのがわかるようになるんです。

武市

ありがとうございます!僕もちょっと違和感があったところが解決したのでありがたいです!


PR

奥野さんがパーソナリティを務める「リファクタリングとともに生きるラジオ」も配信中です。リファクタリングに興味のある方はぜひチェックしてください!

refactoradio.com

© Toreta, Inc.

Powered by Hatena Blog