公開日: 2024/06/10
💬レビュ
普段レビュをする時に考えていることを共有することで、下記を目指したい気持ち。
- PRレビュ時間短縮
- 実装者との認識ずれを防ぎ、PR上での指摘を減らす
- 品質をあげる
- 実装
- 1次レビュ
前提
レビュの目的
- 品質を上げる
- 仕様通りか
- 保守性を高める
- チーム全体の認識合わせ、学習
- （副次的に、特に中規模以上においては開発速度を上げる）
品質が高いとはどういうことか
- エンドユーザが使いやすい
- UI/UX→要件定義フェーズが主
- バグが少ない
- 仕様書通り
- 仕様書での考慮不足/無駄を発見、確認、修正している
- 開発者が使いやすい
- パッと見でそこだけ見ればわかる
- 可読性（コードは書かれるより読まれる方が多い）
- 一貫性
- ステートレス、独立性
- パッと見でそこだけ見ればわかる
実装の流れとおおまかな割合
実装という作業において、実装自体の作業時間はそれほど多くないと思っています（人月の神話参照）。逆にレビュ時に確認事項が発生してしまうとレビュが間延びするため、実装は「確認力」≒「想像力」の問われる作業だと思っています。
- 仕様確認、仕様調整（30%）→超大事！！
- 実装設計（10%）
- 実装（10%）
- 単体テスト、バグ修正（20%）
- レビュ（30%）
レビュをする人が実装者より優れているもの、レビュの必要性
実装者は上記過程を経て、そのタスクに一番くわしい人になっています。では、なぜレビュが必要なのか。
- 複数人で見ること自体に意味がある
- 他チーム含めた全体を見れる
- 客観的に見れる（コード作者の観点から生じるバイアスに影響されないフィードバック可能）
- 実装者にはない技術・経験・知見
レビュの流れ
大きく3フェーズに分けて考えています。
- 基本的な仕様を満たすか
- 構造・意図
- 細部
1. 基本的な仕様を満たすか
- おおまかに仕様通りか
- 仕様はMECEMutually Exclusive, Collectively Exhaustive. = モレなく、ダブりなくに確認できているか
- 明らかなバグがないか
- デザイン通りか
2. 構造・意図
-
PRの分割レビュが遅れると待つ時間が長くなるので分割しずらくなる→まとめる→マージまでが遅くなる→分割が難しくなる…という悪循環に陥りがち
- 単一の目的に絞られているか
- 処理の流れ、構造は適切か
- 大まかな流れが冗長でないか
- 関連要素や前提を減らせないか。そこだけ見ればよいかどうか。
- KISSKeep It Simple, Stupid. / Keep It Short and Simple. = コードをシンプルに保つ。 / YAGNIYou Aren't Going to Need It. = コードは「今」必要なものだけ。 / DRYDon't Repeat Yourself. = コピペ厳禁。
- 宣言的に書けているか
- 分割粒度、責務
- ディレクトリ構成
- パッと見でわかるか
- 命名は適切か
- パッと見で難解な部分はコメントしてあるか
- 見慣れないプロパティなどを使っていないか
3. 細部
- コーディング規約に則っているか
- 細かい書き方
- エッジケース
- エラーハンドリング
- パフォーマンス
- セキュリティ
- ユーザビリティ
その他
- ドキュメントに載せたほうがいい内容がないか
優先度
個人的に重視していること
- わかりやすさ
- 処理フロー
- 私がパッと見でわからなければ、途中参画の新卒は分からない可能性大
- 命名
- パッと見で何をしてるかわかれば読みやすい
- 命名が難しい処理というのは、分割粒度が間違っている場合が多い印象
逆に優先度の低いもの
- かっこよさ
- 仕様通りか
- レビュをする人が細部まで仕様を把握するのは難しく、仕様には実装者が責任を持つべき
- 2次レビュをする人としては全体からみた仕様の整合性（＝一貫性）については実装者より詳しい可能性はある
- マージ速度
- 保守性を上げれば、後半挽回できるし、保守でも楽できる
- 負債は改修されないことが多く、利子が溜まっていく
- コミットメッセージ
- コミット単位ではあまりみていない。修正後くらい？
個人的に気をつけたいこと
- 動いているからOKよりは意味がわかったからOKとする
- 内容は厳しくロジカルに、書き方には気をつける
- 言い方は柔らかく、相手が素直に受け取ってすぐ動けるように
- レビュは指示と受け取られやすく、反発されやすいものだと理解する。特にテキストベースだし。
- なるべく理由を併記する。理由に納得できないと不満がたまりやすい
- あまり具体的な指示は出さないようにしている。それ以外の選択肢が出てこなくなるから
- 良い点も指摘する
- 疑問に思ったら調べるより聞く（時間もないし、実装者の方が詳しいはずだから）
- 意図の確認は積極的にしたい。Question多めだけど煽ってないので！笑
- 指摘よりも質問を心がける
- なるべくLinterで解決する
- レビュを最優先にする
- いろんなチケットを同時に抱えると、コンテキストが増える
- 待っている間にそのPRに影響する変更が差し込まれる可能性
- 完璧なコードは存在しない
- YAGNIを忘れない
やってもらえると助かること
- 実装方針のすり合わせはレビュ依頼前に行う
- 実装してしまってから全体を直すのは大変
- PR上でやり取りするのは大変
- 書く前に構造化
- またはある程度できてからでもいいので再構造化できないか考える
- コードの分割
- 分割できないということは、構造が悪い可能性がある
- レビュの途中にMTGが挟まったりする
- コードの構造を説明する
- 基本的にはコード上に書いちゃうで良いと思っている
- セルフレビュ
- レビュ観点シートは必ず見て欲しい
- PRはレビュをする人に対する発表資料だと思ってほしい
- 構造と意図
- 途中参画の新卒が見てもわかるか
- レビュをする人が他の箇所を自分で参照しなくていいようにする
まとめ
コードレビュはコードの正しさについての万能の解決策でも唯一のチェック方法でもなく、ソフトウェアをめぐるそのような問題に対抗する多層防御のいち要素
- 仕様確認、仕様調整を時間をかけて丁寧に
- パッと見でそこだけみればわかるかどうかは結構大事
- 動いているからOKよりは意味がわかったからOKとする（コードは書くより読まれる方が多い）