「なんとなく」コードレビューしてませんか？
著者：遠山 翔
はじめに
こちらの記事は下記のような人を対象にしています。
- 実装時どのようなことを意識すれば良いかわからない
- コードレビューの必要性がわからない
- とはいえ、チームの決まりだからやっている
- 何をレビューしたらよいかわからない
- 動作確認や関数・変数などの命名指摘より先のレビューができない
ここでは下記について書いていきます。
- なぜコードレビューするのか
- コードレビューにあたり意識すること
- コードレビューのプロセスおよび観点
用語
- レビュイー（Reviewee）：レビューされる人
- 英語として正しいか怪しいですが、記事内ではレビュアーの対義語として用いています
- レビュアー（Reviewer）：レビューする人
なぜコードレビューするのか
そもそもなぜコードレビューをするのでしょうか？私は3つの理由があると考えています。
1つ目は「継続的な開発品質・スピードの向上」です。早期に仕様漏れやバグ、負債になりそうな箇所を解消していくことが、継続的にチームの開発品質・スピードを高めることにつながります。
2つ目は「属人化防止」です。実装されていく機能やコードに対して、特定の人（例えば機能実装者）しか内容を理解していない状態は好ましくありません。例えばAさんが認証機能を実装したとします。その後認証機能の改修やバグの調査が必要になった際、Aさんだけしか対応できないとなると開発が人に依存してしまう（Aさんがボトルネックとなってしまう）ため、開発スピードが落ちてしまいます。理想的にはメンバー全員が対応可能であるべきです。レビューを通して、レビュアーはコードや仕様を理解することで属人性を排除していきます。
3つ目は「メンバーの技術力向上」です。前述の通りレビュアーは指摘に至るまでにコードリーディングをして、コードの良し悪しを判断していきます。レビュイーは指摘を受けて既存のままで良いのか修正したほうが良いのか判断する必要があります。判断に至るまでに自分の中で考えを巡らせ、レビューを通じて新しい考えに触れたり、実装について議論することがチーム全体の技術力向上につながります。
そのため、「デキる人」だけがレビューするのではなく、経験が浅い人も積極的にレビューに参加することが重要です。
コードレビューにあたり意識すること
レビューをする際に意識した方が良いことをまとめました。
レビュイー
- レビューに対して、「とりあえず」修正しない
- 何も考えずレビューを受け入れるということは「業務放棄」です。なぜその修正が必要なのか理解した後に要否を判断しましょう。レビューの内容・意図がわからないときはレビュアーに質問し、理解すること。それがチームの開発品質の向上はもちろん自身の成長につながります。
レビュアー
-
自分のことは棚に上げて指摘する
- 「自分もできてないから」という気持ちを殺してレビューしましょう。前述の通りレビューの目的は「継続的な開発品質・スピードの向上」「属人化防止」「メンバーの技術力向上」です。それらに貢献できるならば、普段自分ができているかどうかは問題ではありません。むしろ指摘事項があるのに指摘しないことのほうが問題です。
- レビューする際は、修正理由も明記する
- 「イケてないんで修正よろしく」は論外です。相手が納得できるよう、どこがどうイケてないのか論理的に説明しましょう。理由を説明できない指摘はイケてないです。
- 指摘対象は「コード」であって「人」ではない
- コードレビューの場では、人への指摘はやめましょう。例えば、「センス無いっすね」「何回言ったらわかるんですか」です。業務上指摘が必要な場合は、コードレビューの場ではなく、別の場所で行いましょう。
- 良い箇所があったらコメントする
- 良い箇所は積極的にコメントしましょう。レビュイーの自信につながります。
コードレビューのプロセスおよび観点
これから私なりのレビューのプロセスおよび観点について書いていきます。
私は3つのプロセスに分けてレビューしています。
1つ目が「内容把握」です。ここではコード・仕様の内容を理解し「レビューできる状態」を作ります。不明点があればレビュイーに質問しましょう。
2つ目が「機能実装のレビュー」です。実装が要件を満たしているかをレビューします。ここに問題があればレビュイーは対応しなければいけません。（要件に対して実装になにかしら不備があることになるので、修正は基本的に必須です。）
3つ目が「コード品質のレビュー」です。挙動が問題なかったとしても負債となりうるコードがあれば、レビューを通してコードを改善します。2つ目のプロセスにて、「要件を満たしていること」が担保されるので、ここの指摘が任意での対応になるケースがあります。チームで「即時対応が必要か」「負債チケットとして後程対応でもOKか」などを議論しながら対応要否を決めます。
下記、各プロセスのレビュー観点の一例です。
1. 内容把握
- 要件（どのような機能が実装されている必要があるか）を把握する
- Pull Requestの内容を把握する
- コードを読み各行がどのように動作しているか理解する
- （必要であれば）ドキュメントが更新されているか確認する
2. 機能実装のレビュー
-
要件と実装内容に差異がないか
- 必要な機能が過不足なく実装されているか
- パフォーマンスやセキュリティ要件が含まれる場合はそれらの確認も含まれる
- デグレしていないか
- 異常系が考慮されているか
-
ユニットテストが実装されているか
- うまくテストが書けないコードは何かしらの問題がある可能性が高い
- テストケースに抜け漏れがないか（正常系＋異常系）
3. コード品質のレビュー
- 不要な箇所（例：入ることのない分岐・デバッグ用のコード）がないか
- 変更により不要になった箇所は削除されているか
- 命名が適切か（変数・メソッド・関数等）
-
（もしあれば）チームのコード規約に違反していないか
- Linterやフォーマッタとして設定されているのが理想
-
必要以上に複雑でないか
- 例：ネストが深くなってきたらガード節（早期return/break/continue）を検討
-
ボトルネックになりそうな処理がないか
- 例：I/OやDBへのアクセス
-
車輪の再開発をしていないか
- 採用しているフレームワーク（例：RoR, Laravel）に機能として提供されていないか
- ライブラリで機能を実現できないか
-
各言語ごとのベストプラクティスな書き方になっているかどうか
- 参考：Googleが出しているスタイルガイド（JavaScript, Python, HTML/CSS, Anguler, etc.）
- 参考：Airbnbが出しているスタイルガイド（Ruby）
- 参考：Effective Go（Go）
- 翻訳版はこちら（翻訳までのタイムラグあり）
-
プログラミングの原則（例：SOLID原則、DRY原則、QCS、YAGNIなど）に沿って実装できているか？
- まずはオブジェクト指向プログラミングの3大概念（カプセル化、ポリモーフィズム、継承）を理解した後に、SOLID原則の学習に入るとスムーズかと思います。
- 原則に従うか否かはメリットと支払うコストのバランスを見て判断しましょう。
- 参考：開発者が知っておくべきSOLID原則 | POSTD
- 参考：CommandQuerySeparation | MartinFowler.com
- 参考：あなたはDRY原則を誤認している？ | Qiita
- 参考：知っておきたい！YAGNIの法則 | Qiita
おわりに
コードレビューの観点を持つことは、レビューはもちろん実装時にもコーディングの指針として、大きく役立ちます。裏を返せば自分が普段意識できていないことは、レビューしようと思ってもうまく指摘できないものです。
今後のレビューはもちろん実装の参考になれば幸いです。
宣伝
パーソンリンクではエンジニアを募集しています！
Discussion