今日から始めるコードレビュー

こんにちは。旅行プラットフォーム部の新卒1年目エンジニアの三浦です。
業務では大きな旅行サイトのプロジェクトに携わっており、技術面・仕事面ともにキャッチアップに追われる日々を過ごしています。

さて、今日は日々の業務で行っている、チーム開発に欠かせないコードレビューについての記事です。
先日チーム内でコードレビューについて話し合う時間があり、その中で勉強になる意見が多かったので自分なりにまとめて皆さんにお伝えしたいと思います。

この記事を読んでくださった方が少しでも「コードレビューやっていくか～」という気持ちになっていただければ幸いです。

記事に出てくる用語の整理

• MR：Merge Requestの略。社内のコード管理に使用しているGitLab上で、改修を行ったブランチをMasterブランチに取り込む際に出すもの。これを見てコードレビューをし、大丈夫そうなら改修が取り込まれる。GitHubのPull Request
• レビュワー：コードレビューをする人。MRを見る人
• レビュイー：コードレビューを受ける人。MRの作成者

そもそもコードレビューってなに？

私は未経験から入社したエンジニアなので、この状態から始まりました。
読んで字のごとく、人の書いたソースコードを見てレビューすることですが、より具体的に言うと、たとえば間違いがないか、もっと良い書き方はないかなどを実装者のコードを確認して指摘したりすることですよね。チーム開発ではそのようにコードの質を担保していきます。

しかしながら、自分でコーディングすることすら不慣れな私には、人のコードを読むことは難題でした。レビューなんて「ベテランの先輩が新人のコードが正しいかどうかチェックする」くらいのものだろうと思い、今のチームに参加した当時は敬遠していました。
ですが実際は様々なメリットが望めるもので、むしろ新人でプロジェクトに途中から参加した私のような立場の人こそ積極的にやっていくべきことなのです。

そう思えるようになったのは、「何のためにコードレビューをするのか」という目的意識を持てるようになったからだと思います。
「バグを減らす」ことももちろん大事な目的ではありますが、それだけではなく様々な目的があったのです。 では、そもそもどうしてコードレビューをするのでしょうか？

コードレビューの目的とは

様々あるコードレビューの目的を整理してみます。 目的が意識できればやる気にもつながりますし、とるべき手段や考えるべきことも見えやすくなります。

バグを減らす

• 仕様の誤解、実装（ロジック）レベルのバグ、改修によるデグレなど細分化もできますが、とにかく予期しない挙動をなくすことです。一番わかりすい目的ですね。
• ある先輩曰く「無くす、ではない（重要）」らしいです。レビューばかりにコストは避けないのでバランスが難しいですね。プロダクトによっても異なりそうです。

メンテナビリティの向上

• メンテナンスのため、可読性向上や実装方針の統一も大事ですね。実装者以外の人が手を入れることが容易になります。
• テストを書いておいて欲しいとなることもありますよね。少し入り組んだロジックでも、テストに通っていればひとまずその単位では動作が保証され、後の調査などの助けになります。

レビュイーの成長

• この部分は特に先輩の方が意識されているところかと思います。実装者にはなかった発想や知識を提示することで、レビュイーの成長となります。

レビュワーの成長

• 先輩曰く「コードを読むことが最大のコーディング力＆仕様理解の向上」。この頃実感できるようになりました。
• 私の場合、理解できないときはまず自分でいろいろ調べてみます。それでも理解できないときはその改修の方針や意図を理解できていないのか、または実装がおかしい可能性もあるので質問してみることに。とても勉強になります。

その他の人の共通理解の助長

• 私のチームではMRへのコメントを全てSlackに流すように設定しています。誰かが気になったり、良いと思ったりした部分が切り取られて流れるので、ここからの学びは相当大きいです。
• まったく触ったことがない部分でも案外、「これ前にMRのコメントでチラッと見たな」となることもあります。

slackにコメントが流れるイメージ

諸説あるとは思いますが、私たちは以上のようなことをレビューの目的として意識しています。
これだけ意義深いとなれば「やってやろう」という気持ちにもなりますよね。 しかし、実際やるとなるとこれがまた大変で難しいのです。私も、自身の成長になるんだ！とは思いつつ、どうコメントすればいいんだ･･･と悩んでしまいがちです。適切なレビューをするにはどのようにしたら良いのでしょうか？

どのような手順でレビューすると良いか

実際にレビューをするには、ただ漠然とコードを眺めていてもだめですよね。レビューをする際に意識すると良さそうな点を順序立てて整理してみます。
これを逆に考えれば、「レビュイーがどんなMRを書いていくと良いか」という話にもつながります。

1. まず改修の目的（before-afterで何を変えようとしているか）を確認

   • 「何のための」「どう考えての」「どういう改修か」を確認する。
   • MRの概要欄や、リンクされている事前のやり取りなどを見ても不明瞭なら遠慮なく質問する。
   • そのMRでは（まだ）やらないことも確認する。不必要な指摘を避けられる。
   • [中上級者向け]レビュイーの仕様認識が妥当そうか検討してみる。
     • プロジェクト特有の知識、コモンセンスなどを頼りにする。
     • どのようにその仕様を確認したか（顧客と直接やり取りした、社内の〇〇さんに聞いた、定義書を確認した、自己判断、など）。

2. 目的を実現する実装方針を自分なりに想像してみる

   • どこで、どんな処理をすることになりそうか。考えた結果見当がつかないとしても考えないより良いはず。
   • 複数案あって良い。
   • 複雑なロジックがありそうかどうか。
   • [中上級者向け]見落としがちな例外ケースに思いを馳せてみる。

3. diffがあるファイルリストをみる

   • diffがあるファイルリストを軽く眺め、ガッツリ見るべきファイルとそうでないファイルにあたりをつける。
   • 複雑な処理がありそうな箇所を見る。
   • テストがあればそこから見た方が楽に実装を追える可能性がある。
   • 2.の自分の想像と乖離があれば注意力をアップする。

4. 実装を追う

   • 感想ベースで、良いと思ったところにスタンプを挟むだけでも良い。
     • ここまで見てもらえているという、コードをレビューしてもらっている感が出てレビュイーも安心できる。
     • 前述のようにslackに流れるので、レビュワー以外への共有にもなる。
     • MRへコメントすることへのハードルが下がって、レビューが活発になる。
   • 読んでもよく分からないところは（調べつつ）率直に指摘する。折角だから教わる機会とするぐらいの気持ちでやる。
   • 1.で確認した目的が過不足なく達成されているか自信がなければ気軽に聞いてみる。
   • 1.で確認した目的と別の目的と思われる改修が含まれていたら指摘する。
   • 2.で想像した方針と実際の実装方針が異なっていたら
     • 自分・レビュイーいずれかに考慮漏れがないか、注意力を上げて見る。
     • 自分の方針の方が良いかもしれないと思ったら、提案してみる。
     • 方針ごとの優劣を考えてみる。どんな基準で方針を決めたか聞いてみるのも良い。
   • 複雑なロジックほど注意してみる。テストが欲しい、コメントが欲しい、などは指摘しやすい。
   • テストケースの過不足やテストコード自体の可読性にも注意してみる。
   • [中上級者向け]類似の仕様や処理、定義値、命名などに心当たりがあれば軽く探して見比べてみる。

どうでしょうか。これだけ順序立てて考えるべき項目を整理しておけば、レビューの質も上がりそうな気がしてきませんか。もちろん、実際にこんなに細かく考えるのは難しいです。それでも、私はこれに即して考えることで以前より楽にレビューできるようになりました。

レビュワーが心がけたいこと

最後におまけとして、チーム全体でより良いコードレビューをしていくために、レビュワーが心がけるべきことをまとめておきます。これはチームや状況ごとに様々だと思います。あくまで私のチームで心がけていることです。

• ある程度時間と労力をかける覚悟をもつ。
  • かけた労力以上の見返りはあるはず。
  • レビューをいっぱいすればチームも活発に、自身もそのアプリのマスターに。
• レビュイーを待たせすぎない。
  • アサインされたら遅くとも1日以内、理想は通知され次第即見るくらいの気持ちで（gitlabの機能で、MRごとに自動で1人のレビュワーがアサインされます）。
  • レビュイーが指摘に対応する時間、も計算に入れる。
  • コメントしたらレスポンスが帰ってきてないか気にかける。
• 本当に見る人を選びそうなら見れそうな人の手動再アサインも視野に入れる。
• レビュイーの労力を減らす。
  • 可能なら改善方法のsuggestを出す。
  • 簡単なコンフリクトなら指摘だけでなく解消してあげる。
• とは言えレビュイーの労力を理由に指摘や質問を遠慮するべきではない。
• ある程度はレビュイーを信頼する。
  • 流石に動作確認はしてるはず、手元で自動テストは通してるはず、など。
  • もちろんレビュイーのレベルにもよる。
• 最悪大事に至らない段階で（CIの自動テスト等で）バグ検出が可能な面については多少注意力を下げても良いかもしれない。
• MRへのコメントはレビュイーだけに向けたコメントではなく、全体への共有のためのコメントでもある。

最後に

先日チーム内でコードレビューについて話し合い、上記の内容が共有されたのですが、以前よりもレビューやコメントが活発になりました。また、実装者もよりレビューしてもらいやすいMR作りを心がけるようになりました。やはり、共通認識は大事ですね。 何より私自身がこれらを意識するようになり、人のMRをよく見るようになりました（とは言えレビューするのは難しいと感じる毎日ですが（笑）)。

MRのコメント欄で議論が繰り広げられている図

チーム開発をする上で避けては通れないコードレビューですが、どうせやるなら楽しく、より成長できるものにしたいですね。それでは、皆さんも良いコードレビューライフを。