はじめに
この記事はDMMグループ Advent Calendar 2023の21日目の記事です。
プラットフォーム事業本部マイクロサービスアーキテクトグループで認可サービスの開発をしているjuve_534です。私が所属するグループではチーム横断でコードレビューを行う仕組みを実施しています。
この記事では横断的なレビューを1年間行った振り返りをまとめています。
レビューシステム概要
振り返る前に、まずは「レビューシステム」について話します。
プラットフォーム事業本部ではGoをバックエンドのデファクト・スタンダードにしています。元々PHPやJavaなどを利用しているエンジニアが多かったため、各チームによってGoの理解度にバラツキがありました。 そこでマイクロサービスアーキテクトグループが横断的にコードレビューを行い、Goの理解度やソフトウェアアーキテクチャに関する支援を行うことになりました。
この取り組みについては上長が登壇した資料「組織のコード品質を向上させる “レビューシステム”の取り組み」があり、取り組みの狙いや成果について話をしているので、ぜひ参照してください。
私は横断的なコードレビューとコードレビューで見つかった課題の共有（上記資料内「issue共有会」）を約1年間行っていました。
うまくできたところ
レビュー支援先の開発チームから悩みを持ち込んでもらえ、ソフトウェアアーキテクチャの悩みを解決できた
コードレビューで見つかった課題をissueに起票していますが、それだけだとissueが対応されませんでした。
起票したissueの中には開発中に取り入れられるような内容もあるので、起票したissueをレビュー支援先に共有する会（通称：issue共有会）を隔週で実施しました。
issue共有会でうまくできたと感じている事例をいくつか紹介します。
Named return value を使う
はじめのうちは私からissueを共有する形で会は進みました。
下記のように会員情報を更新するようなメソッドがあったのですが、返り値がプリミティブのintとなっていました。データの更新処理の返り値としては更新されたID・行数があり得ると思いますが、interfaceをみたときどちらか（もしくはそれ以外か）何が返ってくるかが分かりませんでした。
type User interface {
UpdateUser(userID int) (int, error)
}
そこでNamed return valueを使うことを提案しました。Code Review Commentsでは、戻り値が何か分からないときにはNamed return valueを使うことが書かれていました。Named return valueを使うと返り値に名前をつけることができるので、interfaceをみるだけで返り値のイメージが付きやすくなります。
type User interface {
UpdateUser(userID int) (rows int, err error)
}
レビュー支援先チームに上記の提案を検討してもらいました。結果としてNamed return valueを使うことになり、チームの開発ルールに取り込まれました。
補足ですが、Named return valueのメリット・デメリットに関して弊チームのメンバーが検討した記事「GoのNamed return valueについてメリデメを考える」を書いています。興味がある方はみてみてください。
MVC で C の処理が増えていることに悩んでいる
コミュニケーションの場を設けていると、レビュー支援先から課題を持ち込んでもらえることも出てきました。
MVCのような構造を採用しているチームから「controllerの処理を分けてhandlerで呼ぶ形にするか迷っている」と相談を受けました。
このチームではhttpレスポンスとリクエストをhandlerで受け、ビジネスロジックはすべてcontrollerに書く構造になっていました。例えば、会員情報の更新をして更新後の会員情報を返却するAPIであれば「会員情報の更新」と「会員情報の取得」でcontrollerのメソッドを分けることを考えていました。
上記の実装をしてしまうと、ビジネスロジックを成立させるためにhandlerは複数のcontrollerを呼ぶ必要があり、handlerの責務が増えてしまっています（HTTPのリクエスト&レスポンスの解決とビジネスロジックを成立させるために複数のserviceのメソッドを呼び出す）。
この発想に至った動機を聞いてみたところ、controllerのコード量が増え、対応するテストコードを書く量が増えたことが悩みになっていたようです。それであればリファクタリングするほうが上記の問題への解決策としては適切に見えました。
そこで下記のように提案しました。
- handlerはcontrollerのメソッド一つを呼ぶ形とし、HTTPのリクエスト & レスポンスの解決を責務とする
- 悩みの解決策としては既存コードのリファクタリングを行う
提案内容はレビュー支援先で揉まれましたが、リファクタリングする工数が取れなかったので提案は先送りとし、今まで通りのMVCでhandlerは、controllerのメソッド一つを呼ぶ形で実装を進めることになりました。限られた時間で支援を行う都合上、オーナーシップを持って進めることが難しく、レビュー支援先にオーナーシップを持ってもらっているため、このような結論になりました。
現在はリファクタリングを進めています。
うまくできなかったところ
ドメインに関する悩みを解決することは難しかった
ソフトウェアアーキテクチャに関する悩みは一緒に解決できた一方、ドメインに関する悩みを解決することは難しかったです。例えば、DDDを採用しているチームでドメインモデルの設計がうまくいっておらず、どうしたら良いか相談されました。しかし、普段業務の傍らでレビューしている私はドメインを理解する時間が十分に取れず、悩みを解決できませんでした。
メイン業務とレビューの時間配分が難しかった
業務の傍らで進める仕組みだったので、メイン業務が忙しくなると精神的に余裕がなくなり、レビューを後回しにしてしまいました。結果としてレビューが遅れてしまい、レビュー支援先チームにフィードバックするタイミングが遅くなってしまうこともありました。現在は、レビューシステムを推進するチームができたので、この課題は解決できている可能性があります。
コードレビューを実施したことで得た副産物
プルリクエストのマージ時間が伸びていることに気づけた
コードレビューを行っていく過程で、レビュー支援先チームが抱えている課題に気づけました。
プルリクエスト（以下「PR」）をレビューしていく際に、作られた時間とマージされた時間をメモするようにしていました。これは自分のレビュー実績を示すためにまとめていたのですが、まとめていくと特定のチームのマージ時間が他チームより大幅に遅れていることが分かりました。他チームはおおよそ1日〜3日でマージしていたのですが、そのチームは10営業日以上かかっていました。
気になったので当該チームで課題感を持っているかを確認したところ、当該チームでも課題を持っており、一緒に解決に取り組むことになりました。3カ月ほど一緒に改善活動を行い、マージ時間が3日くらいに改善されました。詳しくは下記2つの記事で話しているので、気になる方は参照してみてください。
- 記事1「プルリクのリードタイムを5分の1に劇的改善。科学的思考とオーナーシップで変革するDMM.comのエンジニア組織」
- 記事2「DMMプラットフォームでプルリクエストのマージ時間を250時間から50時間に減らした話」
最後に
現在、私は認可システムの開発を担当し、レビューシステムは開発者体験の向上を業務とする新設チームが推進しています。この取り組みに興味がある方はカジュアル面談などで聞いてみてください。また認可システムの開発・運用に興味がある方も、ぜひお待ちしています！
以上、最後まで読んでいただきありがとうございました。
