Zenn
👀

私のコードレビューのスタンスと気をつけていること

2024/01/16に公開
#
チーム開発
#
レビュー
#
コードレビュー
tech

これは

コードレビューはソフトウェアの生産活動で最も大切であり、ソフトウェアの開発スキルとコミュニケーションスキルをフル活用する大変な活動だと私は思っています。ソフトウェアの品質とチームビルディングに大きな影響を及ぼす作業にもかかわらず、日常のタスクに組み込まれているため、雰囲気でこなしているとソフトウェアの品質低下やチームの士気の低下を招いてしまいます。

雰囲気でコードレビューをしないための自戒、そしてコードレビューを正しく意見交換をする場とするために、私のコードレビューに対するスタンスと気をつけていることを明記します。

前提

  • 本記事のコードレビューは仕事でのコードレビューを想定しています。個人プロジェクトやオープンソースプロジェクトでのコードレビューについては、本記事の内容が適用できない場合があります。
  • また、チームの規模は小規模（5人以下）を想定しています。大規模なチームやコードレビュープロセスが厳密に決められている組織でのコードレビューについては、本記事の内容が適用できない場合があります。
  • 「高凝集/疎結合になっているか」といった具体的なレビュー項目については、本記事では触れません。

用語の定義

  • レビュイーコードレビューを依頼する人
  • レビュアー：コードレビューを行う人
  • スタンス：レビュイー・レビュアーとしての姿勢や考え方
  • 気をつけていること：レビュイー・レビュアーとしての具体的な行動や方法

レビュイーとして

スタンス

  • Pull Requestの概要は「やったこと」「なぜやったか」「影響範囲」を必ず書く

    • 「コード差分だけでわかるだろう」という考えは怠慢だと思っています。
      • 「いつから自分のレビュー依頼の優先度を上げてもらえると錯覚していた？」
    • 差分が限りなく小さくない限り、No descriptionはレビュー優先度が上がらなくても仕方ないです。
    • Pull Requestの概要を充実することは、現在のレビュアーの認知負荷を下げるだけでなく、git blameなどで後からコードを読みにくる人の認知負荷を下げることにもつながると思っています。

  • レビュー対象のコードの責任は、レビューに通るまではレビュイーが負う

    • レビュアーとしてのスタンス（後述）と逆。
    • まず最低限、Test・Lintが通っていることをCI等で確認してからレビュー依頼をします。
    • 自分でセルフレビュー・セルフチェックをしてみる。「他者のコードに向ける厳しい目を自分のコードに向けていますか？」
      • もっと筋が良い書き方はなかったか？
      • エッジケースでの動作は問題ないか？
      • 表示が崩れていないか？

気をつけていること

  • 「特に見てほしいポイント」を概要に記載します。

    • 例：「〇〇のパターンが全部網羅されているかダブルチェックお願いしたいです」

  • 期限がある場合は、概要に記載する

    • 基本的にコードレビューは非同期コミュニケーションのため、いつレビューするのかはレビュアーに委ねられている。
    • そのため、期限がある・後が詰まっている場合は、その旨を明記することでレビュアーに優先度を上げてもらう。

    • 例：「このPull Requestは、○○の期限が迫っているため、××までにマージする必要があります。」

  • 注意事項がある場合は一番目を引くように記載する

レビュアーとして

スタンス

  • レビュアーとして指定されたPull Requestはリクエストから24時間以内に必ず眼を通す
    • どうしても時間が取れない場合は、Pull Requestの概要にレビュー要件が明記されていればその水準で、そうでなければmustのチェックのみをします。
    • レビュイーにその旨を伝える。
  • レビュイーの書いたコードの責任は、レビューに通った時点でレビュアーもその責任を負う
    • レビュイーとしてのスタンス（前述）と逆。
    • レビューは原因を個人に帰属させないための取り組み。
  • バグや仕様を満たしていない箇所を見つけることを最優先とする。そのような指摘は must(対応必須)ラベルを付し、Request Changesでマージをブロックする。
  • Pull Requestの概要や差分で判明しない仕様や設計に関する指摘は、ask ラベルを付し、質問のコメントを残す
    • そのような指摘は必ずしもマージをブロックするものではないのですが、まれに質問から未考慮の部分が発見されたりするため、レビュイーが回答を得られるまでApproveせずCommentとしてApproveを保留します。
  • 最後に、コードの可読性・設計等の一貫性を向上させるための指摘を行う
    • 趣味嗜好の入る余地がない指摘（例：typo）はnits(細かい、些細な指摘)ラベルを付し、Approveでマージを許可します。
      • 必ずしもmustではないのですが、軽微な指摘であるため修正されることが多いです
    • 趣味嗜好の入る余地がある指摘（例：変数名）や、プロジェクトのスタイルガイド・設計思想として明記されていない事柄に関してはimo(in my opinion)ラベルを付し、Approveでマージを許可します。
      • ※一番誤解が生まれやすいところだと思っています。
      • 大前提ですが、imoラベルはレビュアーの意見を押し付けるものではありません。正解でもないですし、指摘事項に関して必ずしも修正を行う必要はありません。
      • また、コメントに対して必ず返信する必要もありません。「無視された」と負の感情にもなりませんし、全く気にしなくても大丈夫です。
      • このコメントが議論の種になり、より良い設計方針やコードが生まれることもあります。

※見やすいラベルの付け方のテクニックについては、他の方が書かれた参考記事を参考にしています。以下のように使い分けています。
ask-badge
imo-badge
must-badge
nits-badge

  • 常にリスペクトの心を持つ
    • 現実として、基本的に褒めのコメントが指摘のコメントを上回ることはないです。
      • 良いものを作ろうとしているのであれば当たり前の事象
    • レビュープロセスは、レビュイーとレビュアーの時間や労力をかなり消費する取り組み
    • 少しでも良いなと思ったら「:+1:」リアクションしたり、褒めのコメントを残す

気をつけていること

  • コードブロックは必ず対象言語のシンタックスハイライトが当たるようにする
    • OK 
      • func test() {
     fmt.Println("Hello, world")
 }
    • NG 
      • func test() {
      fmt.Println("Hello, world")
  }
  • imo コメントする際は明確な根拠＋「..と自分は感じました/思いました」
    • 必ず主語は自分にする。
  • メモコメントは残さない
    • 備忘や整理のためにメモとしてコメントをするということは、レビュー対象のコードに必要な情報が足りていないということ
    • 必要なドキュメントやコメントを残してもらう・可読性の高いコードにしてもらうよう指摘する

さいごに

GitHub Copilotの登場により、ある程度の水準のコードは自動生成されるようになり、さらにレビューもしてくれるようになりました。私もレビュー対象のコードに目を通す前に、とりあえずCopilotに投げてみることも多くなりました。
とはいえ、チームで作るソフトウェアは生モノです。時期、メンバー、ビジネス都合でさまざまに要件は異なります。この生モノを調理し続けるためには人力によるレビューは不可欠でしょう。

参考記事

コードレビュー観の依代

私のコードレビュー観は、今まで出会った素晴らしいエンジニアのコードレビュー観に強く影響を受けています。

本記事で紹介したテクニック

Discussion