コードレビューをするときに私が心がけていること
ふと、職場に限らず、私がコードレビューをする時、レビューを受けるときに心がけている事をメモしておこうと思ったので、つらつらとまとめます。
1. レビューの目的をはっきりさせる
何の目的があってレビューをするんでしょうか?
(社内の)ルールに則ってコードが書かれていることを確認するためでしょうか?
よりチームの人にとって読みやすいコードにするためでしょうか?
開発言語やプロジェクトで使われていた古いノウハウを誤って使っていないか確認するためでしょうか?
機能が要件通りに実装されていることを確認するためでしょうか?
効率の良いアルゴリズムを採用していることを確認するためでしょうか?
……少なくともひとつ、レビューの目的があるはずです。
最初に、それをハッキリさせます。
職場の場合、(多くは無能な上司や自身に能力があると勘違いしている先輩によって)複数のレビュー目的を課される事がありますが、私の経験から言えば1回のレビューでの視点は、たったひとつに絞るべきです。
つまり、2つの目的があるなら、同じコードを視点を変えて2度レビューします。
3つなら3度、4つなら4度です。
目的の数に応じて、何度でも視点を変えてレビューしていきます。
同時に、以前の(あるいは次の)レビュー視点で押さえておくべきと感じたところは、その回のレビューでは言及せずに、一旦どこかにメモしておきます。
思いついたこと、気になったことは、次のときには全て忘れているというのを心に留めるてレビューをします。
最後にメモを見直して、メモの箇所だけ、ピンポイントで仕上げのレビューをして、全体のレビューを終えるようにします。
2. レビューを受ける相手の熱量を知る
レビューを受ける相手は、いい製品を作るためならコードの書き直しを積極的に受け入れるタイプでしょうか?「せっかく書いたので」といって、稚拙なコードを押し通そうとするタイプでしょうか?
それに対して、(あなたが)そのコードから生み出される価値に対してかける熱量はどうでしょうか?
熱量に大きな差があると、レビューを通じて人間関係に問題が生じる可能性があります。
これは、相手の相対的な熱量が低いほど生じやすい問題です。
コードレビューごときと言っては何ですが、どんな視点であれ、コードレビューは人間関係に軋轢を生じさせてまで無理を押し通すほど価値のあるものではないはずです。
(余談ですがこれに対して、素晴らしいプロダクトを生み出すという過程は、人間関係に軋轢を生じさせても達成する価値のあるものだと思います。多くの場合、その達成の過程で互いに理解し合うことで軋轢は解消されます)
相手の熱量に合わせられるならば、可能な限り相手の熱量に合わせてレビューをします。
レビューの視点から考えて、どうしても譲れないところだけを指摘するのか、レビューの視点から考えて、より良いコードにするためにより多くのコメントを添えるのか、このレビューに対して力をどこまで加えるかは、自分がコードから生み出される価値に対して考えている熱量ではなく、相手の(コードを書いた人、レビューされている人)の熱量に合わせるようにします。
3. コードの向こう側には人間がいる
互いをよく知っている人同士、あるいはレビューする/される行為に慣れた人同士、またはコードから生み出される価値に対して高い熱量で当たれる人同士であれば、忌憚なくコメントを付けていくべきですが、そうでない場合はコードの向こう側に人間が居ることを常に心がけます。
そうです。コードの向こう側は、私達の考えるエンジニアではない可能性があります。
事実、会社員としてプログラムを書いている人の中には、将来に渡って保守しやすいコードではなく、今、その時に動いているコードが正義であると(心の底から)信じている人も居ます。
特に面識のない人やよく知らない相手のコードをレビューをするときは、そういう人がコードの向こう側に居ると想定してコードレビューをします。
他にも、このポイントには心理的なテクニックを使用しないという重要な点も含まれています。
わざと怒りを買うようなコメントをする
コメントに皮肉を加える
極端に偏ったドメインのミームやドメイン知識を持ち出して相手がついてこられない領域で議論しようとする
専門領域の話題を入れて、相手に対しての優位性を見せる
といった行為は、相手との関係がどんなであれ、コードレビューでは絶対にやってはいけない行動です。
相手の感情や心に何かを訴えかけてどうにかしようというのは、ビジネスの領域ではある程度正しさを持つのかもしれませんが、エンジニアリングの領域では全く不要な価値観だと思います。
「エンジニアはマウントを取りに行きがち」なところがあると思うので、自戒を込めて十分に注意したいところです。
また、業界経験の長いエンジニアは、昔話を持ち出した議論も避けるようにしたほうが良いと思います。
先日、実際に動的型付け言語の変数名にiとかstrとか入れているのを見て「ハンガリアン記法みたいだ」という類のコメントをしてしまったところ、ハンガリアン記法が通じなかったために、変にマウントを取りに行ったと相手に取られてしまったケースがありました。
こういうところも気をつけたいポイントだと思います。
4. レビューのはじめに、良いと感じるところを最低ひとつ探します
良いと思ったところに、良いと思った理由を添えてコメントするという事です。
これをレビューの最初にやります。
従って、私がやるコードレビューは、必ず「良いと思うポイントを探す」視点で1回目のレビューを行います。
読んでいて、完全にひどすぎてどうにもならないというケースはほとんど無いと思います。
変数名のセンスでも、アルゴリズムを解説したコメントでも、良いと思えたポイントなら何でも良いのです。
例えば、まともなlintツールを導入できていない悲しい現場で、コミットに合わせてコードフォーマットを整えてくれているとか、残念なテストしか無かった所に、数件のテストを足してくれたとか、そういう雀の涙のような良かったポイントでも良いので、必ずそれを見つけて良いという点を強調してコメントします。
5. そのコメントが何であるのかを明示する
どの視点に照らし合わせたコメントなのかを、相手にきちんと伝えます。
コメントの中には、視点と関係ないものが含まれるかもしれません。
それも、(視点と関係ないという事を)きちんと相手に伝えます。
「コードを見ていて感じた、他プロジェクトのつながり」といったようなつぶやきに近いコメントでも、これから相手がそのプロジェクトに関わる可能性があるならば有用である場合があります。
「レビューコメントを(レビュー視点から逸脱した)メモの代わりに使うな」という意見もあると思いますが、私はコードというある情報の塊を軸にした所に集約するほうが良いと考えているので、現場で禁止されていない限り、細かいメモのような情報も残すようにしています。
また、解決策は提示できないし、表現として的確な言葉を持っていないけれど、経験上「臭さ」を感じるコードに対してコメントする場合に、それが「直したほうが良い」という強い気持ちを含むものなのか、「もう一回だけ考えてみてほしい」という弱い気持ちのものなのか、あるいは相手に対して「経験上臭いと感じた」という事を伝えたいだけなのか、それを明示します。
よく使われるのは、In My Opnion略してIMOというのをつけたり、For Your Information略してFYIというのをつけたり、日本語の表現を使い分けて意図を明示します。
6. 対面でのコミュニケーションは最後の手段
チームや社内でレビューをしている場合、文章をうまく書けないとか、文章だと角のある表現になるからという理由で対面で詳細を解決する方が早いと考えたり、対面で細部のすり合わせをする方が良いと考えて、レビューの合間に対面の話し合いを持ち込むケースがありますが、これはいわば禁じ手だと考えています。
コードに限ったことではないですが、レビューする時に使えるインタフェースはひとつに定められているべきです。
GitHub上のコメントを使ってレビューするのであれば、そのインタフェースを使ってレビューするべきですし、社内のレビューシートのようなフォーマットが存在するのであれば、そのインタフェースを使ってレビューするべきだと考えています。
対面でのコミュニケーションは、用意されたインタフェースを迂回した情報が入ってきます。
それによって視点が定まらなくなったり、飛び火した内容によっておかしな着地を見せるケースがあります。
過去にそういう経験をしたことがあったというのも大きな理由ですが、私はレビューに関しては用意された(単一の)インターフェスを使い、可能な限り対面でのコミュニケーションを避けるようにしています。
7. 相手や他のレビュアーの反応に対しては、可能な限り反応を返する
絵文字や+1のようなチープな反応でも良いので、必ず反応を返すようにしています。
「見ているぞ」「レビューしているぞ」「生きているぞ」というのを、何かにつけて知らせていきます。
ただし、脊髄反射の反応はNGです。
必ず内容を見て、反芻して、何かしらの反応を返します。
内容を見て反芻するので、コメントを通じたまともな反応が返せそうであればそのようにします。
けれども、反応コメントを返すために長い時間を要するようであれば、先述の絵文字や+1といったリアクションだけで反応を返すようにするという事です。
一度反応を返しておくことで、後々その流れから繋がりのあるコードだったり、視点だったり、別のレビューのタイミングでふと思い出して予想していなかった有用な結果に結びつくケースがあるからです。
大半はノイズですが、その中に宝があることを一度知ってしまうと、これを怠るのが難しくなると思います。
以上が、私がコードレビューをするときに心がけている事です。
いずれも1行だけの変更とか、そういったレビューの余地、レビュー実施の余白が殆ど無い場合は当てはまるものではありませんが、コードレビューをするときに、何か迷っている人のヒントになれば幸いです。