Bullet Group Advent Calendar 2020 の 18 日目の記事は水野が担当いたします。
コードレビュー、してますか？
弊社の開発部 (Technology Department) の Ruby プロダクトチームでは喜ばしいことにコードレビューの文化を作り上げることができており、日々愛を込めてレビューし合っています。
時には文字だと感情が伝わりにくく、言葉が強く感じてしまうことがあったため、「アニメアイコンにすれば伝わり方変わるんじゃね？」などと冗談半分で変更してみた結果、二次元の女性キャラクターに指摘されているように感じ取れて気分が良かったと、好感触だったことがありました。1 ※ 個人差があります。
悩まれている方がいらっしゃれば、ぜひお試しください。
実際、捉え方は人それぞれですので、なるべく相手の気分を悪くしないような言葉遣いを特に文字でのやりとりは気を付けています。
このような相手の気持ちを考えて伝えるためにはどうするのが良いとか、表現に関しての記事を書こうと初めは思っていたのですが、最近コードレビューによる進捗の遅れが著しく感じたので、定めた方針について書いていきます。
当然なことを書いていくだけになるかと思いますが、以外に見落としがちだったりするので、ご参考になれば幸いです。
コードレビューの目的
まず目的から考えてみました。
クローズされた PR を見返してみると、「こうすればもっとスマートに書ける」や「こんな書き方どうかな？」などといった書き方のコードレビューが多く見られました。
より良いコードにしたい、可読性を高くしたいと考える IT エンジニアが弊社には多いので、必然的にそうなってしまったのかと思います。
可読性を高くすることはひとつの目的ではあると思いますが、第一のコードレビューの目的にするのは違うと考え、僕は以下のように定めました。
要望通りの仕様になっていること
まずは要望通りの仕様になっていることが大前提です。当然ですね。
仕様と異なっていれば問題になります。
実装者以外にメンテナンスできるようにする
書いたコードの内容をチームメンバーに理解してもらうことは特に重要だと考えました。
他人2が読んですぐに理解できないソースコードは敬遠され、今後メンテナンスされなくなってしまいます。
そのためには、いずれ未来の自分がこのソースコードを改修しなければならないことを意識してコードレビューするべきです。
つまり、コードレビューは改善点などの指摘だけではなく、ソースコードの意図に関する質問もしていくべきだと思います。
バグの指摘を目的にしない
バグの指摘を目的にすると、他の目的の意識が薄くなってしまうと考えました。
指摘をしてはいけないというわけではなく、バグを発見するためのコードレビューはやめるべきだと思います。
バグを探す時間をテストの網羅性を確かめる時間に費やした方が結果的にバグは減っていくと思います。
コードレビューの方法
意識しても実際にコードレビューすると難しい場合もあると思うので、方法もざっくりと考えてみました。
テストコードを読む
テストコードを読むことで、対応した内容がわかるはずです。
コードレビューの最後にもう一度テストコードを読むことで網羅性の確認も行いましょう。
そもそもテストコードの追加・修正がなかった場合は指摘事項になります。
コードレビューに優先度をつける
例えば下記のような問題は最優先にコードレビューするべきです。
- システムや損益に関わってくる問題
- 後から修正するのが非常に困難な問題
このような観点のコードレビューがまだできていない場合は、他の軽微な問題は検出するべきではありません。
もちろん、検知した場合はコメントを残しましょう。
しかし、このように様々な観点のコードレビューをしていくと、どのコードレビューを優先して対応すべきかわかりません。
先頭にラベルを付けてコメントすることで意図がわかるようにすると良いと思います。
- [MUST]
- 絶対に対応して欲しいコメント
- [IMO] (In my opinion)
- 自分ならこう対応するなどといった意見や提案のコメント
- [NITS] (nitpick)
- 細かい指摘などのコメント
- [ASK]
- 質問や確認のコメント
[NITS] が書かれたコメントは低い優先度と捉え、重要な機能でなかったり、時間に余裕がない場合は対応せずにマージを許容します。
ただし、後からでも修正が可能だと判断できたものに限りとします。対応されずにずっと残るやつ
コードレビューの依頼
依頼するときも気をつけなければいけない点がいくつか出てきていたので、この機会に考えてみました。
レビュアーを指名する
僕の所属するチームでは、Slack でユーザーグループにメンションを送ってコードレビューの依頼をしていました。
ですが、基本的に決まったメンバーのみがレビューしていることに気付きました。
そもそも仕様がよくわかっていないので自分がレビューして問題ないのか、誰かが対応してくれるだろうなど、レビューし辛い環境となっていたようです。
このままではよろしくないので、レビュアーを指名する方針を定めました。
レビュアーを指名することにより、自分がやらなければいけない責任感が生まれるのか、コードレビューのまわりが良くなったのでオススメです。
指名するメンバーを決めるのは適当ではなく、チームによって決め方を定めておくと良いと思います。 例えば、僕の所属するチームでは下記のようにレビュアーを多くしないように 2 人で定めています。
- 設計メンバーから 1 人
- あるいは機能に詳しいメンバーから 1 人
- 実装メンバーから 1 人
もちろん、指名されていないメンバーも余裕がある方はコードレビューして問題ありません。
変更ファイル数を少なくする
変更ファイル数が多くなってくるとレビュアーの負担になるだけではなく、時間がかかると判断して後回しになってしまう傾向があるので、なるべく少なくするように努力するべきです。
なるべく細かくチケットを分けたり、機能変更とリファクタリングは分けて PR を作成するなど、変更ファイル数を少なくしましょう。
僕の感覚では、変更ファイル数が 10 以下だとすんなり始められ、15 を超えてくると見て見ぬ振りをしたくなってきますw
作業メモを書く
本当にメモ書き程度で良いので、それなりの規模の回収などの場合は、なにかに作業メモを残しましょう。
どのような意図で変更したのかがレビュアーにわかりやすくなるだけではなく、日を跨いでしまう作業でもどこまで対応したか思い出せる手段にもなります。
概要や背景なども書いておくと、自分がやるべきタスクが見えてくることもあるのでオススメです。
弊社では同様の作業があった場合などにもすぐ参考できるように esa に書き溜めています(\( ⁰⊖⁰)/)
最後に
コードレビューは簡単なようで難しいですよね。
今回書いた内容が最善というわけでもありませんし、またコードレビューによる遅れなどが見えてきたら考える時間を作るつもりです。
この機会に皆さまも再考してみてはいかがでしょうか？
最後までご覧いただき、ありがとうございました！
