こんにちは。株式会社プラハCEOの松原です。
どんな人にこの記事を読んで欲しいか
- コードレビューの効率化に悩んでいる
- コードレビューのやり方に自信が持てず、何か参考になる事例を知りたい
使い捨てコードレビューに翻弄される日々
1~2年ほど前に自社サービスを開発していた頃、弊社では全てのプルリクエスト（以降PR）に対してランダムに割り当てられたレビュワー2名、もしくはテックリード1名にapproveされない限りマージしない運用で開発していました。開発者が5名ぐらいだったと記憶しているので、規模の割にはリッチなレビュー体制だったのではないでしょうか。
修正点があれば指摘して、直して、再確認して、merge。
来る日も来る日も、確認、指摘、修正、再確認、merge。
次第に「僕ら業務時間の大半をコードレビューに使ってね？」と、レビューに費やす時間が気になるようになってきたあたりで、一度自分たちのコードレビューの問題点を考え直す事にしました。
コードレビューが使い捨てになっている状態
- どんなコメントを書くか考えて
- どう伝えるか考えて
- 書く
- 相手に伝わらなかった場合は上記のプロセスを何度も繰り返す
これだけ丁寧なプロセスを経るにも関わらず、PRコメントを見るのは基本的にレビュワーと、その指摘を受けた当事者だけです。我々がPRコメント1つ書くために費やす膨大な時間がマンツーマンレッスンで終わってしまう。非常に時間の使い方の効率の悪い気がしてきますよね...
コードレビューが当事者間で完結してしまうデメリットは他にもあります：
- 別のメンバーや新規参画者が同じミスを繰り返す
- 何度も同じことを指摘していると疲れるので、レビュワーが指摘しなくなる
- 指摘する人が組織を去ると知見も失われ、同じミスが繰り返されるようになる
これはコードレビューがその場に居合わせた当人たちにしか活かされていない状態、つまりコードレビューが使い捨てになっている状態です。
どうやってコードレビューをみんなで共有するのか
とはいえ全てのコードレビューをチーム全体で共有するのではコスパが悪いので、何かテーマを絞って集中的に学習するのがオススメです。PRコメントを全てSlackに流す取り組みも時々目にしますが、他の人のPRコメントまで全て目を通すほど意識の高い人だけの世界だったら警察は要らないんだ...
「みんなで集中的に学習するテーマを探してみよう！」ということで早速今まで交わされた熱いコードレビューの数々を振り返ってみたところ、コードレビューをざっくりと以下の3種類に分類できました：
（WHAT：コードで何を実現したいのか、仕様面に関する指摘）
- 例：このボタンを押した時に動かないよ！
- 例：この機能はこういう仕様になっていなければいけないよ！
（HOW：どうやってコードで実現するか、技術面に関するミクロ指摘）
- 例：Promise.allしたほうがいいよ！
- 例：forEachの中でawait効いてないよ！
（WHERE：どこにコードを書くか、技術面に関するマクロ指摘）
- 例：これはRepository層に書くべきじゃないよ！
- 例：これは別のClassとして切り出したほうがいいよ！
- 例：これはPubSubに任せたほうがいいよ！
- 例：これはマイクロサービスとしてCloud Functionsに切り分けたほうがいいよ！
- 例：これは環境変数に切り分けておいたほうがいいよ！
- 例：これはドメイン層にインターフェースを定義した方が良いよ！
ひとまず今の自分たちは「WHERE（どこにコードを書くべきか）」に関する指摘が非常に多い傾向があったため、これを集中テーマとして取り扱うことを考えました
WHEREは結構厄介な問題
「WHAT」は直さないと動かない（or仕様を満たさない）のでやらなければいけない
「HOW」は修正箇所が限定的なことが多いのでさっさとやっちまえる
なのでまぁPRで指摘されたらすぐ直して終わるのですが、WHEREはなかなか厄介です：
-
直さなくても動いてるのに・・・
（急いで対応する必要がないため、優先度が下がる） -
ええー、今から変更するの大変なんだけど・・・
（テストの書き方、DIの仕方など変更箇所が増えて、工数がかかりやすい） -
うーん、あとで問題が起きてから直せば良いのでは?・・・
（直近のメリットを実感しにくいため納得感が薄い） -
なんでこう書くべきなのか、よくわからない・・・
（アーキテクト経験や、丁寧な設計に基づく実装経験を積んだエンジニアは割と希少なので、本当にその書き方で正しいのか分からない）
なので
- 優先度は低い
- 工数かかる
- 納得感が薄い
- 正しいのか分からない
と、指摘されても対応したくない気持ちが膨らみやすいのがWHEREに対する指摘です。
必然的に「今はやらなくても良いのではないか」「いや、むしろこう書いたほうが良いのではないか」など議論が（PRのコメントも）膨らみやすい傾向がありました。しかもそれだけ時間をかけているのに議論に参加しているのは直接PRにアサインされたメンバーのみ。勿体無い！
全体で共有しよう！
そこでPrAha Inc.では「PR道場」なるものを週次開催するようになりました。説明しよう、PR道場とは！
PR道場：毎週テーマに沿ったPRコメントをメンバー全員に共有して、必要に応じてモブプロして手を動かしながら認識を揃える場のことである
例えば「どこにコードを書くべきか」に関する指摘を誰かが受けたら、毎週1~2時間しっかり時間をとって全員に共有して、手を動かしながら認識を揃えていきます。
特にWHEREに関するテーマは「手を動かしながら認識を揃える」のが重要です。概念だけ覚えても、いざ手を動かそうとすると「あれ、こういうケースってどう書けばいいんだ・・・」と迷い始めるもの。擬似コードでも構わないので、実際に一つユースケースを実装してみると、驚くほど多くの認識違いが浮き彫りになります。自分はDDDを学んだのではなく、学んだつもりになっていたのだなと、この時思い知りました。
僕らの場合、初回は2時間かけて会話しても認識が完璧に揃わず、サイゼリヤで夜ご飯を食べながら議論を続けるような状態でした。各々が過去にDDDに関する書籍などは読了して、それなりの基礎知識は持っていたものの、いざ実装レベルで認識を合わせようとするとこれほど乖離するものかと驚いた記憶があります。
どれだけ議論してアーキテクチャを整えても、全員がそれを実践できなければ意味は無い。それどころか議論に時間を食いつぶすだけで効果が薄い状態になり得る事を学びました。
明文化して、残す
特に新規参画者は新しいアーキテクチャに不慣れなので、こうした議論には置いて行かれがちです。新規参画者には議論を図解して後日発表してもらうなど、整理役をお願いする事でキャッチアップが図れるのではないでしょうか。
弊社でPR道場を開いた際には、新人エンジニアが現行サービスのアーキテクチャとあるべき姿の比較図を書いてくれました。（こちらは初版のため、この後CQSを導入したり、何度か修正を加えています）
こうした図と合わせて、PR道場で話した内容は専用のレポジトリに書き残してVuePressで作った静的サイトとしてFirebaseでホスティングしていました
新規参画者には「とりあえず実装に悩んだらココを見てね」と伝えておけば、ある程度の認識は揃えられます。
ちなみに「ルールに違反するPRを出した人はコンビニにパシらせるぞ！」というルールを作ったところ、僕が真っ先にパシらされました。冬空の下でコーラを買いに走った悔しさ、忘れません。
効果
認識合わせに長い時間を費やしたものの、PRでの指摘に費やす時間が大幅に減り、代わりに祈る時間(仕様ミス等あってはならない失敗を補足する時間的余裕が生まれたりクエリのパフォーマンスについて考える時間)が増えました。体感的に1ヶ月程度のPR道場で元は取れたと感じます。
まとめ
- コードレビューが使い捨てになっていないか？
- 自分たちのチームで共有すべき集中テーマを見つけて、共有してみる(PR道場)
個人的にエンジニアとして一番楽しいのは、勉強熱心な仲間と切磋琢磨しながら新しい事を学び、活かせた瞬間です。こうした組織風土のあるチームであれば、PR道場のような取り組みは有効なのではないでしょうか。学ぶ心がある限り、PR道場は門戸を開き続ける事でしょう！
振り返ってみて
そもそもPRレビューのように非同期＋テキストで行われるコミュニケーションは細かな指摘には向いていますが、すり合わせが必要な情報伝達には同期＋口頭の方が便利ですよね。あらゆるコミュニケーションをコードレビューで完結させようとするから歪みが生まれるのです。コードレビューは銀の弾丸ではないのです って言うとIQが5高く見えるって言われたので書いてみました
「これは非同期＋テキストで話し合うべき内容だろうか？（同期＋口頭の方が良いのではなかろうか）」
という観点を持ちながらPRレビューに臨んでみると新たなアプローチが見つかるかもしれません。
Discussion