こちらの記事は「CBcloud Advent Calendar 2024」18日目の記事です。
もし良ければ他の方の記事もご覧いただけるとエンジニア一同うれしく思います。
はじめに
今のチームにjoinしてから1年半ほどが経ち、エンジニアの数も増えてチームの規模も段々と大きくなってきました。本当にありがたいことです
チームの規模が大きくなるにつれてコードレビューする機会も多くなり、自分が設計・実装するよりも他のメンバーのコードを確認することが多くなってきました。過去の職務経験においてもコードレビューをすることはありましたが、今にして思えば良くないコメントも色々つけてきたなーと痛感させられます
レビューイとなるエンジニアのモチベーションにも影響してくるので、建設的なレビューを心がけていきたいと思う日々です。そんなこんなで今までの振り返りと来年の目標を立てる意味でもコードレビュー時のコメントで自分が実践していること、もしくは実践していきたいことについて改めて明文化しておこうと思います。
コードレビューでコメントをつける時に心がけている(つもりの)7つのこと
指摘箇所についてその理由を添える
コードの良くない記述についてコメントをつける時には必ず「なぜ良くない（と思っている）のか？」を記述するようにしています。
コードを読んでいると「なんかイケてないなー」と感じる箇所が見つかったとします。そのときに特に理由もなく「なんとなく」で修正するように指摘すると、レビューイも納得感がなく「そう言われたから」修正するだけになってしまいお互いに得しない結果となってしまいます1。
きっかけは直感的だったとしても自分が「なぜ『イケてない』と感じたのか」を理由としてコメントにまとめ、根拠となる外部ソース(公式リファレンスなど)があればそのURLも併記します。 もしその理由が思い浮かばないようであれば、思い違いか独りよがりな指摘でしかない可能性が高いためコメントにするのは諦めます。
※ただしtypoなど自明なものについては特に理由も必要ない（はず）なので、そのような指摘については理由もつけずにコメントします。
Bad
このコードはイケてないので修正をお願いします！
Good
https://xxxx (←仕様などをまとめたページ)
上記ページによると〜〜〜という仕様を実装したもののようですが、
このような記述であると本来の意図とは異なる挙動になってしまうように見えます！
質問するときはその意図も明記する
コードの中には「なぜこのような記述になったのか？」が明確でないコードを見つけた際、レビューイにその意図を尋ねることはよくあるかと思います。その時に質問だけのコメントをつけた場合、「レビューイが返答する」→「それを受けてレビュアーがコメントする」→・・・と二度手間になってしまうケースがほとんどです。
このようなコミュニケーションコストを減らすためにも、質問を添える際にはある程度回答を想定しそれを踏まえた改修案も提示するようにしています。
Bad
このコードはどういう意図で記述しましたか？
Good
このコードは〜〜ということを目的として記述したと思うのですが、認識相違ないですか？
それであればXXXXXとしたほうが良いと思います。
可能な限り改修の方向性について案を提示する
コード記述などについて指摘する場合、たいていは「こう直してくれるとありがたいなぁ
」と想定しているものと思います。しかしながら指摘を受けてレビューイに修正してもらった際に、想定外の修正になって返ってくることもしばしばあります。
それを受けて軌道修正のコメント入れて・・・というのも二度手間でしかないため、「こんな感じで直してほしい！」という考えがあるのであれば「自分ならどう書くか？」というコード例を明示的に記載するようにします。
このコードは〜〜という理由でよろしくないため、修正をお願いしたいです。
例えば下記のような感じでしょうか。
ーーーsuggestion
修正後のコード
ーーー
ここで 「この改修案はあくまで修正の方向性を提示するものであり、そのまま取り入れる必要はない」 というニュアンスを含めて伝えることは特に気をつけています。コメントを受けてどのように修正するか(もしくは修正せずにおくか)はレビューイに判断してもらうようにします。
「これは良い！」と思った改修内容はちゃんと褒める
自論ではありますが、ここ最近のエンジニア生活を通じて「レビューとは実装コードに対して良い点・悪い点を総合的に評価する場」と感じるようになりました。「イケてない」ところや仕様通りに動いていない箇所を指摘することも大切ですが、それ以上に素晴らしい実装コードに対してはちゃんと評価することも重要ではないか、と考えています。
「改修箇所について自分が想定していた実装よりも良いと感じたコード」や「自分が認識できていなかったが改修してくれた箇所」があれば、「ナイス！👍」のようなコメントをその評価理由も含めてつけるようにしています2。
このリファクタリングによってコードの見通しが良くなっていいと思います！👍
指摘の温度感を明示する
レビュアーからのコメントに対してレビューイが対応すべきかどうかの判断材料として、「絶対に直すべき」「必要でなければ直さなくても良い」といったレビュアーからの「温度感」も指標の一つとではないかと考えています。ただこの「温度感」はコメントの文面だけから読み取ることはかなり難しいかと思います。
そこでその「温度感」を明確にレビューイに伝えられるようにコメントの先頭に下記のようなアノテーションをつけるようにしています3。
-
[MUST]: 必ず直してほしい箇所 -
[WANT]: できれば直してもらえると助かる箇所 -
[QUESTION]: 実装の意図や仕様について確認したい箇所 -
[NICE]: 良い実装と感じた箇所 - (
[MEMO]: レビューイに向けてのコメント、というよりも自分用に実装内容を把握するためのメモ書き)
[MUST]
今回〜〜という改修をしていますが、それに対応するテストが不足しているかと思います。
XXXXというケースに対してYYYYとなることを検証するspecを追記してほしいです！
「俺が代わりにやる！」は禁句
レビューしていると「自分で書いた方が手っ取り早くない？」と感じることはないでしょうか？少なくとも自分はよくあります。
そういう箇所に対して自分で修正してしまうと自分の作業工数が無駄にかかってしまいますし、何よりレビューイのスキルアップにもつながりません。
なので、あくまで レビューイ自身が修正すること を前提としてコメントをつけることを徹底するようにしています。それでもどうしても自分で修正したい箇所があるのであれば、そこからブランチを切って新たにPRを作る→それを元PRのレビューイにマージしてもらう、といった手段をとっています4。
時には口頭での議論も必要
PRに対して指摘する場合は基本的にはコメントとして残すようにしていますが、時には口頭で仕様や改修方針について会話することも重要かと思います。
たしかに文章という形式で記録が残ることのメリットは大きく、後々言った言わないと揉めることを避ける意味でもコメントをつけておきたいのは事実です。しかしレビュアー/レビューイそれぞれの主張が並行線をたどりコメントの応酬が止まらない、なんてことも稀によくあります。この場合大抵お互いの認識に齟齬があることによるものであるため、口頭で認識合わせをした上でその合わせた内容をコメントとして残しておく、ということで無駄な議論をなくすことにつながるはずです。
また「なんでこんな実装になっているかわからないしその意図も皆目検討がつかない・・・」なんてケースでも、余計にコメントを出し合うことがないように事前に仕様について確認を取るようにしています。
終わりに
あれこれとこうありたいな、という思いで色々書いていきましたが、正直ごくごく当たり前のこともあったかと思います。
私自身今までのレビューを改めて振り返ってみると、実践できているものもあれば正直実践できていないものもありより意識していくことを来年の抱負にしていければ、と思っております。
