🎇華金勉強会 /コードレビュー勉強会

コードレビュー勉強会

なぜやるか

コードレビューに対しての意識が薄い(遅いなど)という指摘を受けたことがあり、コードレビューの目的やよりよいやり方を整理した上で、その課題を解決していけるようにしたい

方法

  • その後、弊社にとってやりやすいやり方を議論する
※変わる可能性あり。
 
 

勉強会の背景

弊社のコードレビューの課題

  • スピード
    • timezoneが違う人が多くレビューが遅い
    • 個々のメンバーが自身の実装タスク重視でレビューは時間ができたらやるぐらいの温度感になってそう
      • それが故にレビュワーの負担を考慮したPRの粒度を考えられていない
  • 内容
    • コードレビューで何を見るべきかわかっていない
    • レビューの出し方/コメントの伝え方
      • なぜそのコメントをしたのかわかりにくいことがある
  • やりかた
    • レビュワーが動作確認までするべきかなどどこまでやるかはレビュワーによって異なっていそうなので、共通認識を作りたい。
 

今回何をするか

Google先生のコードレビューガイドラインを読んだ内容をまとめたものを発表します
https://google.github.io/eng-practices/review/
 

内容

Introduction

  • コードレビューとは、コードの筆者とは別の人がコードを検証するプロセスです
  • コードレビューは、コードとプロダクトの品質を維持するのに行われます。
 

どのようにコードレビューを行うのか（開発者、レビュアー）

コードレビューの基準

  • コードの健全性をあげていくのが目標です。
  • 妥協点のバランスを取っていく必要がある。
 

一番の目的：システム全体のコードの健全性を向上させること

  • 「よりよくする」コードを提出しなければ、コードの健全性は上がらない。
  • もしレビュアーが変更を入れるのを難しくしたら、今後改良を行う意欲がなくなる
  • レビュアーの義務はコードの健全性が低下しない品質であることを確認すること。
    • 時間制約のせいで短縮する場合、ちょっとずつコードの健全性が低下しがち
  • レビュアーはオーナーシップと責任感をもってやらねばならない
    • 一貫性、メンテナンス性等がコードに維持されるのを求められる
  • PRが完璧でなくても、作業中のシステム全体のコードの健全性を確実に向上させるのであれば、承認するのを優先すべき
    • ただし望んでいない機能が追加されていたりする場合は承認を拒否できる
  • 必ずしも「完璧」でなくても、「よりよい」コードであれば良い
    • 「細部の完璧」を求めて、時間がかかるのは本末転倒である
    • あまり重要でない修正点の場合は’Nit:’等前置きをおこない、「無視しても良い改善点」として伝えるようにしましょう。
  • システム全体の健全性を確実に悪化させるPR を正当化しません。緊急時のみです。
    • 緊急時に関しては、まず緊急事態を解決できているかを最優先し、解決後に徹底的なレビューを行うべきです

メンタリング

  • 知識を共有することはコードの健全性を向上させる方法の一部
  • だが純粋に教育的なもので、コード全体の健全性の向上にそこまで寄与しない場合は、’Nit: ’等の前置きをして、解決が必須ではないことを示すべき

原則

  • 技術的事実、データ＞個人の嗜好、意見
  • スタイルに関しては、スタイルガイドを絶対とする。
    • 逆にガイドにない部分は作者のものを受け入れる
  • ソフトウェア設計は基本的な原則に基づくべき
  • 他のルールが適用されない場合、現在のコードベースに合わせることを依頼できる。
  • レビュアー、開発者でこのレビューガイドを元にして合意形成を図っていくべき。
    • 合意形成が困難な場合、コメントのみでなく対面の会議等で解決を図るべき
      • その場合コメントに経緯と決定事項を記載しておくこと
    • それでも合意できない場合はより広いチームでの議論、他のメンテナ等に判断を仰ぐようにする

開発者側（PRを出す方）

良いChangeLogの記述方法（開発者側）

※あくまで Googleでのルールです。
ChangeLogはどのような変更がなされたか、なぜ行われたかを示す公開記録です。今後システムが続く限り参照される意識を持って作成しましょう
  • 最初の行で、何をするかの要約を書く（バージョン管理の履歴サマリに表示される）
    • わかりやすく、実用的であること
    • 英語の場合は命令形で記載する
  • 2行目から数行空白をあける
  • 本文は詳細な情報提供
    • 変更リストを全体的に理解するために必要な補足情報を書く
    • 解決しようとしている問題の説明
    • なぜこのアプローチなのか
      • アプローチに欠点が想定されるならそれも記載する
    • バグ数、ベンチマーク、設計文書へのリンク、背景情報等
    • 外部リソースへのリンクを含める場合は、将来の読者が閲覧できない可能性にも配慮すること。できれば、PR内で理解が完結するようだと好ましい

不適切な記述

  • 「何を」「何故」「どうやって」変更したがわからない記述
    • bugfix
    • build修正
    • フェーズ1
    • 変なURLを消す

小さくてシンプルなPRにしましょう

  • レビュースピードが早い
    • ❌レビューに30分必要な1つの大きなPR
    • ⭕5分で読める小さなPR複数
  • 徹底的なレビューができる
    • 大きいPRの場合、指摘箇所が多いので大量のやりとりがおこり、不満を感じやすい
  • 変更箇所が少ない＝バグ発生が少ない
    • 変更箇所が少ないため、確認もしやすい
  • PR却下時の無駄な作業が減る
  • マージがしやすい
  • 小さい方が設計がしやすい
  • レビューでブロックされることが少ない
  • ロールバックが簡単
  • 「PRが大きすぎる」という理由で、レビュアーは変更を完全に拒否する裁量がある。
    • PRが難しすぎる場合に、開発者はそれを適切に直す必要がある

小さいPRの最低要件

  • 自己完結していること
    • ただ一つに対処する、最小限の変更をする
    • 小さすぎるPRと大きすぎるPRの許容できる範囲をレビュアーと開発者の間で同意をとる
  • テストコードをすべて含む
  • 現状の変更でレビュアーが理解する必要のあることはすべてPRに書かれていること
  • 理解しづらいほどちいさくはしない
    • 新しいAPI追加の場合は、使用方法を記載する
  • 「大きすぎる」の厳密なルールはない
    • 100行は妥当
    • 1000行は大きすぎる
    • 影響ファイル数にも配慮すること（1ファイル200行は問題ないかもしれないが、50ファイルにまたがると通常大きすぎる）

レビュアーコメントへの対応

  • 個人攻撃に受け取らない
    • 批評は個人とそのの能力に対する攻撃ではなく、個人、コードベース、プロジェクトへの助け
  • レビュアーのフラストレーションがコメントで現れてることがあるが、その中から「伝えようとしている建設的なことはなにか」を自答して行う
  • コメントに怒りのままに反応するのはエチケット違反！
    • レビュアーが建設的・正しくない方法でフィードバックしていない場合は、直接そのことをレビュアーに伝えること。
      • 直接伝えても意図した効果が得られない場合は適宜さらに上司にエスカレーションすること
  • レビューはレビュアーとの共同作業。
    • 開発者はコメントを観たときに「レビュアーが求めていることを理解しているか」を考える
      • わからない場合はレビュアーに質問する
      • 理解しながらも同意できない場合は、協調的なコメントを残すよう心がける。何故同意できないかを説明して、レビュアーがどうするのを求めているかを聞き直す。
      • レビュアーが知らないことを開発者が知っている可能性もあるので、その情報もあたえての議論が必要。
  • コンフリクトを解決する
    • 開発者とレビュアー間でのコンセンサスを得ることが大事。
  • 礼儀、敬意は常に最優先されるべきものです！

コード修正方法

  • コードそのものを明確にする
  • なぜそのコードがあるかを説明するコメントを追加する
  • レビュアーがコードの一部を理解できなかった場合、未来の読者も理解できない可能性が高いので、コメントをしておく必要がある

レビュー観点

最初にサマリ

  • コードがよく設計されていること。
  • コードがよく設計されていること、コードの利用者(開発者、エンドユーザーともに)にとって機能が優れていること。
  • UI の変更が適切であり、見た目が良いこと。
  • 並列プログラミングが安全に行われていること。
  • コードが必要以上に複雑でないこと。
  • 将来必要になるかもしれないが、今必要かどうかわからないものを実装していない。
  • コードが適切なユニットテストを持っている。
  • テストはよく設計されている。
  • 開発者はすべてのものに明確な名前をつけている。
  • コメントは明確で有用であり、ほとんどが「何を」ではなく「なぜ」を説明している。
  • コードが適切に文書化されている（一般にg3docで）。
  • スタイルガイドに準拠している。
  • あなたがレビューを依頼されたコードのすべての行をレビューすること、文脈を見ること、コードの健全性を向上させること
  • 開発者の良いところを褒めることを心がけてください。

レビュー観点詳細

設計全体

  • コード同士のまとまりに問題はないか
  • 機能を追加するタイミングとして適切か

機能

  • 求めた通りの機能が実装されているか
  • その機能はエンドユーザー、開発者にとって良いことか
  • 開発者は正しく動作するようテストをしておくべき
  • レビュアーはエッジケースを考え、コードを読んだだけではわからないバグがないかを確認する必要がある
    • 開発者に機能のデモを提供してもらうのもあり
  • レビュアーは並列プログラミングのデッドロック、レースコンディションが考え抜く必要がある

複雑さ

  • 必要以上に複雑なコードになっていないか
    • 「コードを読む人がすぐに理解できるか」
    • 「呼び出しや修正を行うとバグが発生しやすいコードになっていないか」
  • オーバーエンジニアリングに注意する
    • 「将来解決する必要があるかもしれない」と推測している問題は今解決しない
    • 今解決すべき問題のみを解決する

テスト

  • 変更に適したテストが必要
    • 緊急事態以外は必須です
  • 正しく、有用であることを確認すること
  • シンプルで有用なアサーションか
  • メソッドごとのきりわけができているか

命名

  • 適切な命名はできているか

コメント

  • 明確なコメントを書いているか
  • すべてが必要がコメントか
    • 「なぜ」このコードが存在するかを説明する。「何をしているか」は書かない
    • 「何をしているか」のコメントがないとコードが読めないのであれば、コードの方を簡単になるように修正すべき
      • 正規表現、複雑すぎるアルゴリズムの場合はのぞく
    • ドキュメントとはことなるコメントは異なり、そのコードがどのように使われるべきか、どう動作するかを表現するものでなければならない

スタイル

  • 適切なスタイルガイドに従っていることを確認する
    • スタイルガイドにないものを指摘するときは「 Nit:」をつけ、修正が必須でないことを開発者に知らせる
      • 個人的な好みでPRMergeを阻害しない
    • 大きなスタイル変更を他の変更と組み合わせない。
      • reformatは別PRにする

整合性

  • 既存コードとスタイルガイドが矛盾している場合は、新規で作成するコードはスタイルガイドに従うべき
    • 要求事項でなく推奨事項を表しているガイドの場合は、局所的な矛盾があまりにも混乱を招くときは既存コードに従ってもいいが、基本的にはスタイルガイドに従うようにする。
    • それ以外は基本的には既存コードとの一貫性があるようにする
  • いずれにせよ既存コードの整理をするためのTODOを作成すべき

ドキュメンテーション

  • ビルド、テスト、操作、リリース等の方法を変更する場合、README等の参照ドキュメントも更新されているかどうかを確認していること。
    • 削除や非推奨の場合も、ドキュメントの削除をおこなうかを検討すること
    • 欠落している場合はドキュメントの追記、生成を依頼すること

全行見る

  • 開発者が直接書いたものは基本的にすべての行を見ること
    • 自動生成コード等は全部読む必要はない
  • 中身が問題ないと決めつけずに読む
  • コードを読むのが大変でレビューが遅くなるなら、開発者にその旨を伝え、開発者は読みやすいコードに書き換える
    • レビュアーが理解できない場合は他の人も理解できない可能性が高い。将来の開発に支障がでるので、もっと別の書き方を模索する
  • 複数のレビュアーがいて、一部のみをレビューする場合はどの箇所をレビューしたかを記録し、コメントつきのLGTMを行う。他のレビュアーが他の部分をすべてレビューしたときに初めてPRをMergeすることが可能です
 

コンテキスト

  • できれば修正箇所を広く見る
    • 50行のメソッドの中に4行追加するなら、より小さなメソッドとして切り分けたほうがよい
  • システム全体の文脈からPRを考える
    • 健全性が損なわれていたり、複雑性があがっていたりしないか。小さな変更が組み合わさることで複雑化するので、新しい変更で小さな複雑化を防ぐことが重要。

良かったことを伝える

  • 励まし、感謝を伝えるとメンターシップの点からいうと価値がある！
 
 
 

レビューの仕方

  1. 変更は理にかなっているか？適切な説明が書いてあるか？
  1. 変更の重要なところを最初にみる。よく設計されているか？
  1. 残りを適切な順序で見る
 

コードレビュースピード

  • 高速であるべき！
    • チーム全体のベロシティ低下
    • 開発者側のフラストレーションにつながる
    • コード全体の健全性に影響が与える
      • 良くないPRを出す圧力が高まる
  • 目安としては最大一営業日以内にすべき。（翌朝一番に対応）
    • 典型的なPRは一日に数回レビューを受けられるはず。
  • スピードと中断のバランスとしては、レビュアーの区切りの良い点を待つ（会議が終わった後など）
  • PR提出後の応答時間が大事です。個々のレスポンスのスピードを大事にしてください。
    • プロセス自体に長い時間がかかっても、素早い反応があればフラストレーションはかなり緩和されます
    • レビュアーが忙しい場合は以下のような対応もありです
      • 開発者にいつごろ対応できるか連絡する
      • 別のレビュアーを当てる
      • 大まかなコメントを最初に残す
  • コメントでのLGTMをする
    • コメントが残っていたとしても、適切に対応してくれる場合はしてしまってOK
    • 指摘があっても軽微なものである場合は出してもOK
  • そのためにも巨大なPRは小さなPRにすべき
 
 

ネクストアクション

  • 現状PRは作成者とレビュワーしか見ないので、知見になりそうなレビューがあった場合に共有できるような仕組みを作りたい
    • stamp押したらslack　@Yuki Fukatsu
  • PRのテンプレはどこかで議論したい　各チームごと定例で話し合い
    • 「別PRで対応する項目」は追加してもいいかも
  • コードレビュースピードについてルールを各グループで決める　各チームごと定例で話し合い