simon

コードレビューガイド - コードレビュー規範#

作成日: 2021 年 6 月 22 日 17:47

Google Code Review Practice と Zaihui Practice に基づく

用語 Term#

説明を簡略化するために、用語の略語を統一します。

• TLDR - Too long, Don't Read 太長不読のバージョン
• Author - この PR の著者 コード作者
• CR - コードレビュー コード審査、略してレビューとも呼ばれる
• PR - プルリクエスト、コード統合リクエスト、変更リストを含む
• Reviewer - コードを審査する人
• WIP - 作業中 開発中であり、統合できない
• Comment
  • 本文には 2 種類のコメントがあります
  • コード内のコメントは注釈です
  • CR プロセス中に残されたコメントは改善提案です
• Code Base - コードベース
• Deadline - 締切
• Judgement Call - エンジニアの良心に基づく個人的な選択
• LGTM - Look good to me 見た目は良い
• NIT
  • NitPick
  • 些細なことを指摘すること、無視できる改善提案を指します
• SG - スタイルガイド、コードスタイル規範
• UT - ユニットテスト
• Emergency - 緊急事態、一部の CR 基準を放棄することができます
  • Hotfix
  • 不合理な締切が発生した場合でも、締切後には欠落部分を補う必要があります

目的 Purpose#

• 高品質で迅速なコードレビューを行い、コードベースのコード品質を継続的に向上させる
• コードを読むことやコメントを残すことで、新しい言語、フレームワーク、デザインパターンなどの知識を教えたり学んだりする

原則 Standard#

• コードベース全体のコードの健全性を保証する
• 原則 1 を満たす場合、可能な限り PR を承認する、たとえそれが完璧でなくても
• 継続的な最適化は完璧さよりも重要
• 可能な限り多くの改善提案コメントを残す、改善が必須でない場合は 'nit:' で始めて必須ではないことを示す
• レビューコメントはできるだけ具体的な改善案を提供するべきだが、著者がより良い案を見つけることも奨励するべき
• 緊急 PR でない限り、PR はコードベース全体の品質を損なうべきではない。緊急の定義については後述します

重点 What to look for#

概要 Summary-TLDR#

• コードは良い設計が必要
• 機能性はコードの使用者に優しくあるべき
• すべての UI の変更は合理的かつ良好でなければならない
• すべての並行プログラミングはその安全性を保証する必要がある
• コードは可能な限りシンプルであるべき
• 開発者は今必要でない機能を事前に書くべきではない
• コードには適切な UT が必要
• UT は良い設計である必要がある
• 開発者はすべてのものに簡潔で明確な名前を付けるべき
• コードのコメントは簡潔で有用であり、理由を説明するべきで、何であるかを説明するべきではない
• コードは適切に文書化されるべき
• コードはコードスタイル規範に従うべき

設計 Design#

• 他のコードとの相互作用の設計は合理的か？
• 統合システムとの統合性はどうか？

機能性 Functionality#

• PR の著者の目的は何か？
• この目的はシステムにとって良いのか悪いのか？
• 並行プログラミングの必要性と、デッドロックや競合状態を引き起こす可能性があるか。デッドロックや競合を引き起こす並行モデルの使用はできるだけ避ける。

複雑さ Complexity#

• PR は必要以上に複雑か？
• 定義
  • 現在の設計がこれらのコードの使用や修正を容易にバグを引き起こす場合、その設計は過度に複雑である
  • 過度設計
    • 開発者がコードを必要以上に汎用的に設計する
    • またはシステムが現在必要としない機能を追加する
    • 将来の機能はできるだけ将来に解決するために残しておくべきで、実際にその要求が必要になったときには、関連する客観的な環境やシーンが変わっている可能性があり、事前に書かれたコードが適用できないことがある。
    • PS: 事前に開発する必要はないが、将来の要求に対して設計上の余地を残すことは必要である
• 分析の観点
  • 独立したコード行
  • 関数
  • クラス

テスト Testing#

• 通常、テストは追加された機能コードと同じ PR 内に存在する必要があり、緊急事態を除く
• テスト設計
  • 基準
    • 正確
    • 合理的
    • 有用
    • 有効
  • 考慮すべき問題
    • データ準備は実際の状況と完全に一致しているか？部分的なデータだけを構築しない
    • すべての成功パスがカバーされているか？
    • すべての 400 の可能性に対してテストが行われているか？返されるエラーメッセージは期待通りか？
    • 成功したリクエストは、返されるデータが一致するだけでなく、データベース内のデータも正確であるか？
    • コードが破損した場合、テストは正常に失敗するか？
    • コードが変更された場合、虚偽の正解（false positives）が発生するか？
      • 例えば、UT を通過させるために特別なデータを設定し、表面的には UT が通過するが、実際にはバグがある
    • 各テストには簡潔で効果的な検証があるか？
    • テストは合理的に異なるテストメソッドに分割されているか？
    • テストもコードであるため、主要なブランチでないからといって不必要な複雑さを許可してはいけない

命名 Naming#

• 開発者はすべての命名が良いか？
• 良い命名には
  • それが何であるか、または何をするかを説明するのに十分な情報を含む
  • しかし、長すぎて読みづらくならない

コードコメント Comment#

• 原則
  • 簡潔
  • 理解可能
  • 必要
• 内容
  • コードが表現できない内容
  • 出現すべき内容
    • 決定の背後にある理由
    • なぜこれらのコードが存在するのか
    • todo 未来にやるべきこと、todo で始まるコメント
  • 出現すべきでない内容
    • コードが何をしているかを説明すべきではない
      • コードの機能説明はコード自身が表現するべき
      • コードが十分に簡潔でない場合、その機能を説明するために文字による説明が必要であれば、コードは簡素化されるべき
      • 例外：正規表現や複雑なアルゴリズムは詳細なコメントによる説明が必要
    • コードの使い方や表現を説明すべきではない
      • これは文書が行うべきこと

スタイル Style#

• 社内では、使用される主要な言語ごとに強制的に統一された合理的なスタイル規範（style guide、SG）が必要
• SG は絶対的な権威です。PR は SG に従うべきであり、SG は PR に適応すべきではありません。
• SG に存在しないコードスタイルの改善を提案したい場合は、「nit」を付けて必須ではないことを示すべきです。個人のコードスタイルの好みによって PR の承認速度を遅らせるべきではありません。
• 必須のスタイルは SG に追加されるべきです
• 大量のコードスタイルの変更を行う場合、必ず別の PR を提出し、他の機能コードと混合してはいけません。

一貫性 Consistency#

• PR は SG に一致しているか？
• SG の内容が推奨であり、強制ではない場合、変更は Judgement call に依存するか？
  • しかし、コードの可読性が低下する場合を除き、SG に従うことを優先するべきです

文書 Document#

• いつ文書が必要か？
  • PR がコードの使用、テスト、相互作用、またはリリースプロセスを変更した場合
  • PR がコードを削除または非推奨にした場合、関連文書の削除を検討する
  • 文書が欠落していることに気付いた場合、原著者に文書を追加するよう要求してください！
• 修正が必要な文書
  • README
  • すべての関連文書

各行 Every Line#

• レビューを担当するコードの各行を読むこと
• スキャンできる内容
  • データファイル
  • 生成されたコード
  • 大規模なデータ構造
• スキャンできない内容
  • 手書きのクラス、メソッド、コードブロック
• 一部のコードはより多くの注意を必要とする
  • これはレビュアーの判断に依存する
  • あまり注意を払いたくないコードについても、少なくともそのコードが何をしているかを理解している必要がある
• コードが長すぎたり複雑すぎてレビューの速度や理解の速度を妨げる場合
  • すぐに PR の著者に知らせるべき
  • PR の著者がコードを簡素化するか、コードの役割を明確にするまで、コードレビューを続けるべきではない
• これらのコードを理解しているが、評価する能力が不足している場合
  • 自分が十分な能力を持っているレビュアーを招待し、コードレビューに参加させることを確認する

コンテキスト Context#

• 追加されたコードブロックはしばしば部分的なロジックであり、コンテキストのコードと組み合わせて読む必要がある
• コンテキストが少ない場合、コードレビューツールで展開して確認できる
• コンテキストが複雑すぎる場合、コードをローカルにダウンロードし、IDE や他のツールで確認する必要がある

良い実践 Good Things#

• 通常、レビューで残されたコメントは改善提案の批判です
• しかし、コードの中に素晴らしいものを見つけた場合は、コメントで感謝の意を表し、PR の著者に感謝し、より良い実践を使用するように励ますことをためらわないでください。
  • 例えば：「素晴らしい！」
  • 例えば：「このコードは非常に巧妙に書かれていて、学びました」
  • 賞賛の言葉に加えて、具体的な賞賛があればさらに素晴らしいです。著者と共鳴することができます。

PR の見方#

概要 - TLDR#

• 私たちの PR 規範に合致している
• この PR の変更は合理的か、PR が不合理な場合は即座に却下する
• PR の主要部分の設計を確認し、設計上の問題があれば即座にフィードバックする
• コードの論理チェーンの順序で残りの部分を確認する

ステップゼロ：必要な規範に合致しているか#

• 関連する Jira チケットがあるか
• コミットメッセージが規範に合致しているか
  • Jira-ticket(tag): description
  • tag:
    • feat 機能
    • fix 修正
    • ut ユニットテスト
    • mi 表の変更
    • refactor リファクタリング
    • inf インフラ
• 目標ブランチにリベースされているか

ステップワン：変更を大まかに理解する#

• PR の説明を確認し、大まかに何をしているかを理解する
• 変更自体は意味があるか
• 不合理に感じた場合は即座にフィードバックする
  • フィードバック時は、まず相手の作業を肯定し、その後受け入れられない理由を述べ、次に何をすべきかを提案する
• 継続的に不合理な PR が発生する場合は、開発プロセス（チーム内部および拡張チーム）を反省する
  • これらの不合理な PR が開発前に停止されるようにする

ステップツー: PR の主要部分を確認する#

• 主要なコード論理ファイルを見つける
• 見つからない場合は、著者に尋ねるか、著者に PR を分割するよう要求する
• 主要な設計問題を最初に確認し、問題が発生した場合は即座にフィードバックし、現在の CR を停止する
  • 一般に、開発者が PR を提出した後、開発を続けることが多いが、この PR に基づいて開発が行われる場合は、早急に停止し、損失を最小限に抑える必要がある
  • 設計問題は修正に多くの時間を要し、大部分の機能には締切があるため、早めに修正を提案することが締切に間に合う助けとなる

ステップスリー：合理的な順序で残りの PR を確認する#

• PR の論理チェーンを見つける
• 論理チェーンに従ってレビューする
• まず UT を読んで、設計理念を理解するのが良い

迅速な CR#

概要 - TLRD#

• 忙しくても迅速にフィードバックを行う
• 最長フィードバック時間は 1 営業日以内
• もし集中して作業している場合は、手元の作業を中断しないでください
• 大規模な PR は小さな PR に分割されるべきで、分割できない場合でもステップワンのレビューを完了する必要がある
• コードの複雑さが CR の速度を低下させる場合は、著者に簡素化を求めるか、説明を提供する必要がある

理由#

• 遅すぎる CR はチームの効率を低下させ、他の人の開発を妨げる
• 遅すぎる CR は開発者が CR プロセスに抵抗を感じる原因となる
• コードベースのコード品質に影響を与える
  • 一方で、コードを効果的に統合できない
  • 他方で、遅すぎる CR は開発者の開発品質を低下させる

どのくらいの速さが必要か？#

• 集中してタスクを行っていない限り、CR リクエストを受けたら即座にレビューを開始するべき
• 最長フィードバック時間は 1 営業日を超えてはいけない

速度 VS 中断#

• 中断されないことを保証することが CR の速度よりも重要
• 中断から集中状態に戻るには多くの時間が必要
• 集中を中断するポイントを選び、CR を開始する

迅速な応答#

• 忙しくても、迅速な応答を持つべき
• 本当に非常に忙しい場合は、まず大まかな意見を提供することができる、つまり上記のステップワンのことです

時差を超えた CR#

• 相手の退社前に CR を完了させることを保証しないと、CR プロセスは時差によって大幅に延長される

コメント付きの LGTM#

• LGTM は統合可能であることを意味する
• 注意すべきは、LGTM の本来の意味は、このコードが **「私たち」の基準に合致していることであり、「私」** の基準ではない
• 時には LGTM にもコメントがあるが、承認には影響しない
  • レビュアーは開発者に信頼を寄せ、後でこの問題を解決することを知っている
  • 残りのコメントは重要ではなく、解決する必要はない

大規模な PRs#

• 著者に PR を小さな PR に分割するよう要求する
• 分割できない場合は、全体の設計を確認し、ステップワンを実行する
• コード品質を低下させる前提で、開発の次のプロセスを妨げないようにする

著者が CR で行うべきこと#

Jira チケットを関連付ける#

• PR の内容は Jira チケットと一致する必要がある
• 異なるタスクが発生した場合は、新しいチケットを作成して関連付け、別の PR を提出する

フィーチャーブランチで提出する#

• 自分のリモートで主要なブランチと同名のブランチを使用して CR を提出すべきではない
• alpha-only や free-only のような不安定なブランチでテストを提案すべきではない
• 自分のリモートで、ターゲットブランチをコードベースとして基にしたフィーチャーブランチを作成して開発と CR を行うべき

コードレビューの観点から自分の PR を見直す#

• 詳細は第 4 節を参照

スモークテストを完了する#

• CR リクエストを提出する前に、自テストを完了する必要がある。
• テストの専門家がスモークテストのチェックリストを提供するので、すべてを完了する必要がある。

コミットメッセージの規範を遵守する#

• 1 つの PR、1 つのチケット、1 つのコミット
• Jira チケットを関連付ける
• WIP のプレフィックスを削除することを忘れない
• フォーマット：第 5 節のステップゼロを参照

パイプラインを通過させることを保証する#

• コードスタイル
• テスト
• ビルド

必ずレビューが必要なアサイナーを指定する#

• 新しい機能ロジックの場合、著者の他にバックアップの主要レビュアーが必要で、同じロジックを知っている人が 1 人以上いることを保証する
• 古いロジックの変更については、元のコードの著者にレビューを依頼する必要がある。なぜなら、彼らはこの部分のビジネスロジックや歴史的な問題をよりよく理解しているからである。元著者の情報は git blame を通じて知ることができる

CR の進捗を追跡し、必要な支援とフィードバックを提供する#

• CR を提出したら、著者は PR が迅速にコードベースに統合されるように推進する義務と責任がある
• コードの論理チェーンが長い場合、PR の説明や直接レビュアーに説明する方法で、レビュアーが主要な論理チェーンを迅速に理解できるように助けるべき。ただし、レビュアーの中断をできるだけ減らすように注意し、レビュアーが CR を開始することを示したときにのみ口頭で説明する。
• レビュアーからのコメントにはすべてコメント形式で返信し、修正するかどうかを返答する必要がある。修正しない場合は合理的な理由を提供する必要がある。レビュアーがまだ同意しない場合は、オフラインで議論するか、第二のレビュアーを引き入れて仲裁する必要がある。
• PR が CR 提出後 1 営業日以内にレビューされていない場合、翌日の朝会で提起する必要がある。

PR 統合後は、できるだけ早くテストを提案する#

• PR の統合は、このコードがテストプロセスに入ることを意味する
• フロントエンドとバックエンドの共同開発の場合、一般的にはフロントエンドがテストを提案する。純粋なバックエンド開発機能はバックエンドがテストを提案する。
• ただし、段階的にテストを提案する原則に基づき、バックエンド PR の統合後も単独でテストを提案することができる。この点についてはテストと事前にコミュニケーションを取って確認する必要がある。

レビューの開始#

原則：

• レビュアーの精神状態は完備であるべき
• レビュアーは心流の作業状態にあってはならない
• レビュアーと著者は効率的に相互作用できる

結論：

• CR には固定の時間が必要
• 夕食後の 1 時間を推奨