PRレビューを自分や相手のために快適にしよう

CARTA fluct エンジニアのなっかー @konsent_nakka です。

PRをレビューする時、レビューされたPRを見る時、どんな辛さを感じていますか？

レビューする時

PRが大きくて脳のメモリに収まりきらない
1つのPRで複数のことをやっていて、変更箇所がなんのために変更されているのかわからない
タイトルやdescriptionに変更意図を読み解くために必要な情報がない、または整理されていない
- 結果、何をやりたいPRなのかすんなり分からない
レビューコメントの意図が過不足なく伝わるか心配

レビューされた時

細かい質問や指摘が多くて、変更や返答に時間がかかる
修正箇所が多すぎて元々のPRと全然違う形になったり手戻りが多い
どのくらいの重要度で指摘されているのか分からない
- 次のPRで対応すれば良いのか、今すぐ直して欲しいのか、どっちでも良いけど別解を示しているだけなのか

本記事の範囲

レビューする時に困っている人

レビューされる側として困っているときは、↓を参考にしてみると良いかもしれません。

https://techblog.cartaholdings.co.jp/entry/recommend-pr-self-review

PRレビューでやること・やらないこと

PRレビューでやること

まず、タスクの解決策として正しいかを確認することが大切です。
コードを書く際には、「そもそもなぜこのタスクを行うのか」「このタスクを実行することで得られる利点は何か」「この解決手法が最適であるか」という点をクリアにしてから、解決手法の1つとしてコードを書き、PRを出すようにしましょう。人間は目先のことに囚われがちで、How（コードでどう書くか）ばかりに目がいって、前提の部分を忘れがちです。
そのため、課題を解く理由、解くと嬉しい理由、解決方法まで妥当であることまでレビューで確認してあげられると最高です。
ただし、これを実現するためには、PRのタイトルやDescriptionをちゃんと書いてレビュアーに伝えることが必要不可欠です。なお、PRの書き方については、ここでは扱いません。

次に、コードのシンプルさや意味が伝わるかどうかを確認することも重要です。具体的には、変数や関数の命名が誤解なく明快に伝わるか、不要なネストが含まれていないか、設計として分割すべきか、抽象化すべきか、純粋であるべきか、などを確認します。

PRレビューでやらないこと

前提として、レビューでPRの動作に対して責任を持つ必要はありません。
ただし、レビューされる側が動作チェックを希望する場合もあるため、そのような要請があれば動作検証も行います。
しかしながら、本番環境にリリースした後に動作しない、あるいは操作の考慮漏れがあったなどの問題が発生した場合、その責任はレビューされる側が負うべきです。

可能な限り早い応答を心がける

レビューは最速で返すことが重要です。この点については、人によって様々な意見があり千差万別なので一概には言えませんが、強いチームや名著では素早い応答をするべきとされている傾向にあります（私の観測範囲では）。

CARTAでは、できる限り他者の仕事をブロックしないようにするため、比較的に自分の仕事を中断してもレビューを返している傾向にあります。この考え方は、エクストリームプログラミング本やプルリクのレビューは即返すといった資料にも見られます。

一方で、google-engineering-practicesでは「もしあなたがコードを書くなど、1つのタスクに対して集中状態にある場合には、コードレビューをするために自分の集中を妨げてはなりません」としているチームもあります

PRのタイトル、Description、紐づくIssueを読んでタスクの背景を理解する

PRのレビューを行う際には、まずタスクの背景を十分に理解することが重要です。PRのタイトル、Description、そして関連するIssueを読み込み、それらを理解しましょう。ポイントは以下を見ることです

そもそもなぜこのタスクを行うのか
実行することで得られる利点は何か
そして選択した解決手法が適切であるか

これらの点を見極め、方向修正が必要だと判断した場合はそれを伝えることが大切です。

ただし、全ての内容を完全に理解することは現実的ではなく、見落としがあることは避けられません。それでも、気づいた時点で方向修正を手伝うことができれば大丈夫です。

このプロセスを効果的に進めるためには、レビューされる側の協力も不可欠です。PRを作成する側は、タスクの背景や目的、選択した解決手法の理由などを明確に記述しましょう。

コメントでメモを書く

PRのレビュー過程において、コメントでメモを書くことは非常に有効な手法です。

まず、リーディングした情報をコメントとして書き出すことで、頭の中の情報を外部に追い出し整理することができます。

メモを書く際は、それがメモであることがわかるような印をつけることが重要です。例えば、GitHubを使用している場合、:memo:でメモアイコンになります。これにより、通常のコメントとメモを視覚的に区別可能です。

このようなメモは、自分自身のためだけでなく、他のレビュアーにとっても有用な情報源となります。複数の目で確認することで、レビューの質を向上させることができます。

さらに、メモの解釈に誤りがある場合、他のレビュアーや開発者から指摘を受けることができます。これは、チーム内での齟齬を減らし、共通理解を深める良い機会となります。

質問する

コードを書いた人は、そのコードについて深い理解を持っているかもしれません。しかし、レビューする側は必ずしもそうではありません。異なる視点や経験を持つレビュアーが疑問に思う点を質問することは、他の開発者や将来のメンテナンスを行う人々にとっても同様に疑問となる可能性が高いです。

また、分からないことをきちんと伝えることは、チーム内のコミュニケーションを円滑にする上でも重要です。自分が分からないことを率直に質問することで、他のメンバーも同様に疑問点を気軽に共有できる環境が作られます。逆に、分からないことを伝えずに黙っていると、他の人も分からない時に質問しづらくなってしまう可能性があります。

単に自分の理解を深めるだけでなく、チームの知識共有、心理的安全性の向上にも繋がります。

PRを分けるべきか考える

PRのサイズや範囲について適切に判断し、PRのサイズや範囲を小さく保つことは効果的なコードレビューを行う上で重要な要素です。ただし、PR作成者以外が正確に判断するのは、実際にはかなり難しい場合が多いのも事実です。

PRを評価する際には、以下のような点を考慮する必要があります。

まず、影響箇所が多すぎて、予期せぬ問題が発生するリスクが高くなっていないかを検討します。変更の範囲が広すぎると、思わぬ副作用を引き起こす可能性が高まります。

次に、該当のPRによって意図しない挙動が引き起こされた場合に、適切に対応できる単位であるかを確認します。具体的には、revertなどで簡単に切り戻せるか、また、問題が発生した際に原因を特定しやすい単位になっているかを考慮します。

さらに、PRが理解可能な範囲に収まっているかも重要です。これはレビュアー側の感想になりますが、PRが大きすぎるとレビューが困難になり、見落としも増える傾向にあります。そのため、できるだけ小さな単位に分割することが望ましいというのが本音です。

PRが大きすぎると、レビューに時間がかかり、結果としてリリースやフィードバックのサイクルが遅くなります。これは、チーム全体の生産性低下にも繋がります。

指摘コメントには対応して欲しい度をつける

PRのレビューにおいて、指摘コメントに対応の優先度や重要度を明示することは、レビューされる側のハンドリングを容易にする上で非常に有効です。例えば、nits、imo、mustのような表現を使用することで、コメントの重要度を明確に伝えることができます。

この方法を採用する理由の一つは、対応レベルを明確に伝えないと、ちょっとしたぼやきのような軽いコメントまでも「絶対に対応しないといけない」と受け取られてしまう可能性があるためです。

個人的には、:memo:、[q]、[nits]、[imo]、[must]という5段階の表現を使用しています。それぞれの意味は以下の通りです：

:memo: : 個人的に脳のメモリに入らなかった情報をコメントで注釈入れるイメージです。これはレビューされる側に直接投げているコメントではないため、形式を明確に区別しています。
[q] : シンプルに質問したい時に使用します。
[nits] : 細かい点で、対応してもしなくても良いが、対応するとさらに良くなる可能性がある場合に使用します。必要であれば別PRでの対応も問題ありません。
[imo] : 強い事情や意思がない限り対応してもらった方が良い場合に使用します。
[must] : 絶対に対応して欲しい強い気持ちがある場合に使用します。例えば、対応しないとインシデントが起きかねない場合や、チームで決めた絶対にブレてはいけない方針に反している場合などです。ただし、この表現の使用頻度は比較的低いです。

このように、コメントの重要度を明確に示すことで、レビューされる側は優先順位をつけて効率的に対応することができ、また、レビュアーの意図も正確に伝わります。これにより、PRのレビュープロセスがよりスムーズになり、チーム全体の生産性向上にも寄与するでしょう。