いい感じのコードレビューしたい、されたい。

こんにちは、ネットオン開発Sec.のNです。

弊社開発Sec.では、コミットされたコードは必ずコードレビューを受けます。

今回は、コードレビューをする側、される側双方で心掛けるといいと思うことについてまとめました。

※ あくまで、こうしたほうが良さそう、という推奨です。

レビューされる人が心掛けるといいこと

以下のようなことに気を付けると、レビューしやすいです。

コミットは小分けにしたほうがいいです。

これは印象ですが、開発経験豊富な人ほどコミットのサイズが小さいです。
逆に、経験が浅い人ほどひとつのコミットが大きく、全部まとめて一気にコミットしてしまいがちです。
作業の全体像が見えていないから試行錯誤しながらの開発になってしまい、そのせいで切りのいい作業単位でコミットできないからではないかと思いますが、少しづつコミットしていれば、一定の作業単位で変更を取り消したりできるので、試行錯誤も楽です。
ひとつのファイルへの変更を複数のコミットに分割したい場合は、git add -pを使うといいかもしれません。

必要に応じてDraftとして作業途中でもPRを出してレビューを受けましょう。

実装方針の修正などが容易になります。
その場合は、すでに実装が終わっている箇所、まだできていないところ、課題などが分かるようにPRのコメントに書いておきましょう。
あまりに差分が多いと、広く深いレビューができなくなります。

コミットメッセージはちゃんと書いたほうがいいです。

その変更がどんな意図に基づいてなされたかを知りたいので、「バグ修正」とか「レビュー指摘対応」だけで終わらせるのはあまりよくありません。
また、コミットメッセージは複数行書くことができます。
２行目を空行にして、３行目以降に詳しい内容を書くことができるので、長くなりそうならそちらに書いてください。
プレフィックスを付けると、一目でコミットの意図を理解しやすくなります。
Conventional Commits などを参考にしてください。↓ こういうヤツ

fix バグ修正
hotfix クリティカルなバグ修正
add 新規ファイル追加
feat 新規機能 / feat は feature の略
update バグではない機能修正
change 仕様変更による機能修正
refactor リファクタリング修正
disable コメントアウト等で無効化する
delete/remove ファイルを削除・コードの一部を取り除く
rename ファイル名の変更
move ファイルの移動
upgrade バージョンアップ
docs ドキュメントのみ修正
style 空白、セミコロン、行、コーディングフォーマットなどの修正
perf 性能向上する修正 / perf は perfomance の略
test テスト追加や間違っていたテストの修正
chore ビルドツールやライブラリで自動生成されたものをコミットするとき

コミットに余計な変更を含めるのは望ましくないです。

「面倒だし、ついでにこのコミットにまとめて入れちゃおう」というのは、ちょっとした変更についてやりがちですが、よくないです。。
もしその変更を cherry-pick したり revert することになったら、混乱の元になります。

変更の意図が伝わるようにしましょう。

何をしたのか、何をしないのか、ちゃんとコメントを書きましょう。
何がしたいのか分かっていればその方法の是非について考えるのは比較的容易ですが、意図がよくわからない変更をレビューするのは難しいです。
あなたが自分が書いた内容を理解しているのなら、意図を明確にするのは簡単なはずです。

その他、PRのコメントとかでもいいので、役に立ちそうなことがあれば書き残しておきましょう。

開発中に議論に上がったことなどあれば、簡単にでも書いておいてください。
チャットとかに情報が散らばって、後から見たときに何が何やら分からなくなりがちです。
おそらくですが、あなたのコミットはあなたが退職した後も残ります。
１年後か２年後か分かりませんが、あなたとは違う別の誰かがあなたの書いたコードを見て「なんじゃこりゃ？」とならないように、参考になりそうなことは書き残しておきましょう。

レビューで指摘された内容に盲目的に従う必要はありません。

結局のところ、あなたの修正内容についてもっともよく理解しているのはあなたであるはずです。レビュワーが的はずれなことを言っている可能性も（当然！）あります。

納得いかない場合は議論しましょう。

レビューする人が心掛けるといいこと

強い言葉を使うのは止めましょう。

感じ悪いです。
また、否定的なコメントは結局耳を貸してもらえません。
Microsoftもそう言っています。
(引用) 作成者が非常に否定的なコメントを有用だと感じる確率は 57% ですが、中立的なコメントを有用だと感じる確率は 79% でした。
Characteristics of Useful Code Reviews: An Empirical Study at Microsoft

変更を依頼するなら、具体的に指摘するほうがいいです。

「これイケてないよねー」とか言ってレビューした気になるのは止めましょう。

変更が必要だと思うなら、具体的に「ここをこう変更してください」と書きましょう。

皮肉、嫌みは止めましょう。

そんなことを言うくらいなら黙っていた方がましです。

修正が必要なのか、単なる意見なのか、質問なのか、はっきりしましょう。

レビューの意味合い、温度感が分かるようにしましょう。
コードレビューラベルを使うといいかも知れません。↓ こういうヤツ

[must] 必ず直してほしい部分に使う
[should] 可能であれば対応する。
[nits] 些細な指摘であることを伝える
[imo] 個人的な意見であることを伝える
[ask] 質問事項、確認事項を訊く
[fyi] 参考までに情報提供のみ
[typo] タイポの指摘
[suggestion] 単なる提案

もめたらミーティングとかをセッティングしたほうがいいです。

時々ずーっとだらだらPRのコメント欄で喧嘩して（議論して）いるのを見かけますが、そんなところでやり取りしても合意に至ることは不可能なので、さっさと別の場所をセッティングしてください。
Googleもそういっています。

（引用）Don’t let a CL sit around because the author and the reviewer can’t come to an agreement.
resolving conflicts

変更箇所だけを見てレビューするのはよくないです。

変更箇所以外も見たほうがいいです。変更忘れとかあるかもしれません。
できればローカルで動かしましょう。
変数名、関数名の変更による参照エラーなどは、差分だけでは分からないことが多いです。

分かりにくい、理解できないというのも、重要な指摘です。

あなたにとって理解が難しいコードは可読性、メンテナンス容易性も低い可能性が高いです。
あなたが理解できないということは他のメンバーも理解できないのかも知れません。
（だからと言って「これ何がしたいの？」とか言ってはいけません。）

常にナイストライの精神を忘れないでください。

たまには他人を褒めましょう。

便利かもしれないツール

GitHubのPRページでレビューするのではなく、VSCode上でレビューすることをオススメします。
以下の拡張機能を導入すると、ローカルでのレビューが便利になります。

まとめ

自分のことを棚に上げていろいろと書きましたが、気持ちよく開発し、気持ちよくレビューしあうには、上記のようなお約束は大切だと思います。

レビュワー、レビュイーは同じゴールに向かう仲間だということを忘れず、いい感じにやっていきましょう。

Post Views: 2,723