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

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

作成日: 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 の主要部分の設計を確認し、設計上の問題があれば即座にフィードバックする
コードの論理チェーンの順序で残りの部分を確認する

Step zero: 必要な規範に合致しているか#

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

Step one: 変更について大まかな理解を得る#

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

Step twoの主要部分を確認する#

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

Step three: 合理的な順序で残りの PR を確認する#

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

迅速な CR#

总结 - TLRD#

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

原因#

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

应当多快？#

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

速度 VS 打断#

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

快速响应#

忙しくても、タイムリーな応答を持つべきである
本当に非常に忙しい場合は、まず大まかな意見を提供することができる、つまり上記の step one である

跨时区 CR#

相手の退社前に CR を完了することを保証する。そうでなければ、CR プロセスは時差によって大幅に延長される

LGTM with comments#

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

大型 PRs#

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

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

关联一个 Jira ticket#

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

以 feature branch 提出#

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

以 Code Review 的角度审视一遍自己的 PR#

詳細は第 4 節を参照

完成冒烟用例自测#

CR リクエストを提出する前に、自テストを完了する必要がある。
テストの専門家は冒煙用例チェックリストを提供し、すべてを完了する必要がある。

遵守 commit message 规范#

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

保证 pipline 通过#

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

指定必须要 review 的 assigner#

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

跟进 CR 的进度，提供必要的帮助和反馈#

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

PR 合入后，应尽快提测#

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

レビューの開始#

原則：

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

結論：

CR には固定された時間が必要である
夕食後の 1 時間を推奨する