Help us understand the problem. What is going on with this article?

コードレビュー虎の巻 〜指摘の数だけ強くなれるよ〜

More than 1 year has passed since last update.

コードレビューされる側からする側になって1年あまり。
普段意識していることをまとめます。

はじめに

チームのルールに従う

チームの数だけ文化があります。
新しく結成されたチームでも、途中からjoinした場合でも、そのチームのルールが最優先です。
一般的なプラクティスや自分の考えとは異なっていても、独自の判断で無視しないようにしましょう。
「悪法もまた法なり」です。(もちろんルールのメンテナンスは必要です)
また、ルールがない場合は定めましょう。

目的を見失わない

コードレビューの目的はソフトウェアの品質を向上することです。
「レビュアー vs レビュイー」ではなく、「問題 vs 私たち」です。

「良いコード」を知る

レビューする側が指摘するにも、レビューされる側が実装するにも、「良いコード」とは何かを知ることが必要です。
書籍や記事がたくさんあるので読んでおきましょう。
リーダブルコード』『リファクタリング』あたりは鉄板です。
最近のだと『プリンシプル オブ プログラミング』『現場で役立つシステム設計の原則』あたりも読みやすくておすすめです。
手前味噌で恐縮ですが、私がまとめたものもあります。(追加や修正などのリクエスト歓迎です!)
コーディングの指針

観点

設計

  • 各種原則に沿っているか
    • KISS
    • DRY
    • YAGNI
    • PIE
    • SOLID
    • 安定依存の原則
  • 結合度が低いか
    • 他のモジュールへの依存が最小限になっているか
  • 凝集度が高いか
    • 関連する機能が1ヶ所にまとまっているか
  • クラスやメソッドが大きすぎないか
    • 目安はクラスが300行以下、メソッドが20行以下
  • インターフェースがシンプルか
    • 引数の数の目安は3つ以下
  • 状態がシンプルか
    • 変数の数を減らす
    • スコープが大きくなるほど特に注意する

実装

  • 要件・仕様を満たしているか
  • 仕様に漏れがないか
  • スコープが適切か
    • 変数やメソッドを必要以上に公開しない
  • ネストが深くないか
    • 目安は3以下
    • 早期リターンやメソッドの分割によって減らす
  • パフォーマンスの問題はないか
    • 必要十分なクラス・メソッドを使う
    • 適切なアルゴリズムを使う
      • むやみに多重ループしない
    • 適切なデータ構造を使う
      • 配列、連結リスト、集合など
    • 外部へのアクセスを最小限にする
      • IO
      • DB
      • Web API
  • セキュリティの問題はないか
    • パスワードや秘密鍵を含めない
    • 外部からの入力はバリデーションおよびサニタイズする
  • 条件を網羅したテストがあるか
  • 不要なコードやコメントが残っていないか

スタイル

以下のように、自動化できることはレビューで扱わないようにします。

  • スペース
  • 改行
  • 1行の文字数
  • インデント
  • シングルクォート/ダブルクォート

以下の点はレビューで扱います。

  • 命名が適切か
    • 具体的な名前をつける
    • 重い処理を行うメソッド名にはget/setを避ける
    • 短縮しない
    • その言語の一般的なケースにする
      • 定数はアッパースネークケース、クラス名はパスカルケースなど
  • 適切なドキュメンテーションコメントがあるか
    • JSDoc(JavaScript), JavaDoc(Java), docstring(Python)など
    • 「何をするクラス・メソッドか」「どう使用するか」を書き、「実装の詳細」は書かない
  • typoがないか

バージョン管理

  • コミット粒度が適切か
    • 1コミットで1種類の変更のみにする
    • 1レビューの目安は500行未満
  • コミットメッセージが適切か
    • 何をしたか明確にする(追加、更新、削除、整理、バグ修正など)
    • チケットやIssueがある場合は対応する番号を含める

レビュアーが意識すること

レビューのリクエストには最優先で対応する

レビューが完了するまでレビュイーのタスクが止まります。
遅れれば遅れるほど、結果的にチーム全体のスケジュールも遅れることになります。
また、レビューがたまるとコンフリクトが起きる可能性も高くなります。
新しいコードを書く前にレビューを終わらせましょう。

指摘が多くなる場合はまず高いレイヤーの部分だけ行う

レビューする観点には設計から変数名までいくつかのレイヤーがあります。
高いレイヤーの修正によって細かい部分も変更されるため、最初から命名に関するコメントをしても無駄になってしまいます。

You-MessageではなくI-Messageでコメントする

コメントをやわらかくするため、You-Message(非難、命令)ではなくI-Message(感想、提案、質問)を使いましょう。

  • :x: 変数名が不適切です
  • :x: ○○してください
  • :o: 変数名がわかりにくいと感じました
  • :o: ○○はどうでしょうか?
  • :o: この処理を入れているのはなぜでしょうか?
  • :o: 私なら○○にするかもしれません

理由を説明する

修正の提案をする場合は理由も説明しましょう。
レビュイーに納得して修正してもらうには、その提案がより良い方法である理由が必要です。

原則に基づいてコメントする

提案の理由を説明する際に、当てはまる原則がすでにある場合はコメントに含めましょう。
原則やデザインパターンは問題解決の手段としてだけではなく、開発者間の語彙としても使えます。

コードの例を挙げる

プログラマーに意見を理解してもらうにはコードの例を挙げるのが最も早いです。
ただし、些細な修正に対しても毎回挙げるのは煩わしくなるので気をつけましょう。

厳密にしすぎない

重箱の隅をつつくようなコメント(nitpick)をつけすぎないようにしましょう。
軽微な内容であれば、場合によってはそのままマージすることも検討すべきです。
レビュイーのモチベーションが下がったり、チームとしての開発速度が落ちる可能性があるからです。

良いと思ったこともコメントする

チームのパフォーマンスを上げるためには、メンバーのモチベーションを高く保つことや協力的な関係が重要です。
修正すべき点だけでなく、良いところもコメントしましょう。
以下のケースは称賛するチャンスです。

  • 自分が知らなかった便利なAPIを使っていた
  • 複雑だったコードがシンプルになった
  • ドキュメンテーションコメントがしっかり書かれていた

コメントの仕方が難しい場合はGoodだけでも十分です。

レビュイーが意識すること

コードを自分と切り離す

レビューでついた指摘は自分に対するものではなく、コードに対するものです。
必要以上に固執せず、柔軟に受け入れましょう。

方針の時点でリクエストを作成する

実装がすべて終わってからではなく、方針(設計)が決まった時点でリクエストを作成する方法があります。

  1. 方針を記載したMarkdownをコミットに含めてリクエストを作成する
    • その際、コミットメッセージやリクエスト名の最初にWIP:[WIP]とつけておくと作業中であることが明確になります
  2. レビュアーがMarkdownに対してコメントする
  3. レビュイーはレビュアーのコメントに応じて方針を修正する
  4. 2, 3を繰り返し、方針が承認されたら実装に着手する
  5. 実装完了後、コミットメッセージやリクエスト名のWIPを外す

この方法のメリットは以下です。

  • 実装前にレビューできるので手戻りが少なくなる
  • Markdownにラインコメントをつけられるので、逐一引用する必要がなくなる
  • 方針の変更履歴がdiffになるので追いやすい

参考: https://blog.shibayu36.org/entry/2016/08/05/103000

Issueのリンクを貼る

リクエストのDescription(もしくはコメント)にIssueやチケットのリンクを貼りましょう。
レビュアーは必ずしもそのリクエストの背景を知っているわけではありません。
むしろ基本的には知りません。
方針や設計を共有するためにもリンクを貼るとスムーズです。

変更前後のキャプチャを貼る

UIの変更はコードだけではわかりにくいので、キャプチャ画像があると丁寧です。
変更後だけでなく、変更前のものも並べるとより丁寧です。

参考

akira_
Why not register and get more from Qiita?
  1. We will deliver articles that match you
    By following users and tags, you can catch up information on technical fields that you are interested in as a whole
  2. you can read useful information later efficiently
    By "stocking" the articles you like, you can search right away