良いコードレビューとは

コードレビューする時、自分がどんなことに気を付けているか (本当は気をつけたいか)みたいなポイントをまとめてみた。

コードレビューの目的

• プロダクトの品質を担保するため
  • 人は基本的にミスをするもの
  • 1人で考えたものより、2人、3人集まって考えたものの方が良いことが多い
• 知識をチーム内でシェアするため
  • チームでコードに関する知識を常に共有し続けることで、「この機能はAさんしか知らない」といった属人化問題を防ぐ
    • Aさんが有休取った時に限って障害が起きたりするんですよね。分かります
  • 他の人が書いたコードを読み、さらに分からないことは質問できる、素晴らしい学びの場だと捉える
• 責任をチーム内でシェアするため
  • 何か問題が起きた時に関連するコードを書いた人間だけが責められるようなことは決してあってはならない
  • レビュー時 (又はそのコードがデプロイされるまで)に問題に気づけなかったチーム全体の責任なので、チームで振り返り、解決していく

Reviewer編

レビューする時に気をつけたいポイント

• PRで何をレビューするべきかを理解したか
• コードは該当IssueのAcceptance criteriaを満たしているか
  • UIが絡むものはReviewerもローカルで動作確認するのが望ましい (QAで担保するなどやり方はチームに依るとは思う)
• 設計は問題ないか
  • 拡張性はあるか
    • 将来的な仕様変更によって容易に破綻しないか
    • Disposableな部分であればあとで何とでもなるが、プロダクトのCoreとなる部分(DB設計とか)は特に慎重に設計レビューする方が良い
  • クラスやメソッドの責務は適切に分離されているか
    • テストしやすいコードになっているか
      • テストが書きにくい -> テスト対象のクラスやメソッドの責務がそもそもおかしい場合が多い
        • 依存が多すぎないか、色んなことをやろうとしすぎていないか
        • 例えば、a_and_bみたいなメソッド名が出てきたら明らかに一つ以上のことをやろうとしているので、責務を分離できることが多い
  • 依存関係が怪しくないか
    • ModelがViewに盛大に依存しているなど
    • Goとかだと循環参照したらコンパイラに怒られたりするよ
  • エラーハンドリングは適切に行われているか
    • 若干極端な例だが、APIで明らかにクライアントエラーなのに、サーバエラーを返しているとか
    • エラー情報から問題を正しく理解できるようになっているか
• テストは十分書かれているか
• バグはないか
• デグレはないか
• パフォーマンスなど、非機能要件の問題、懸念はないか
• 変更をデプロイする際にメンテナンスやダウンタイムは発生しないか
  • 発生する場合はデプロイフローはちゃんと提案されているか
• セキュリティリスクはないか
  • 気づくの難しいけど意識はしておく
• コーディングスタイルはチーム内のルールに沿っているか
  • 基本的にReviewerではなく「Lintでチェック -> 修正が必要ならCIで落とす」のように自動化しておくのが良い

マインド面

• 自分が後にそのコードをメンテする前提で臨む
  • 「これ、自分が今後メンテするのツラそうだな」と思ったら、未来の自分のためにもRevieweeと協力して全力で改善する
• 「自分がもし実装するならどのようにするか」という観点でレビューする
  • 自分の設計と相手の設計を比較することで、レビューしているだけで設計力が磨かれるし、より建設的な議論ができるようになる
• 良くない実装を見つけても、人ではなくコードを指摘する
  • 指摘する際も「なぜ良くないと思うか」必ず理由をつけて指摘する
    • 例えば「この設計はxxxのようなケースでxxxのような問題が起こる可能性がありませんか？」のように、「自分もこのコードを良くすることに協力したい」という姿勢が伝わると良さそう
    • 自分がされたら嫌だと思うような言い方は絶対にしない
    • 自分の指摘が誤っていた場合は素直に謝る。そして議論してくれたことに感謝する
• 理解できない部分も積極的に質問する
  • 質問からミスに気づけることもあるし、問題がなかったとしてもReviewerは学べる。Revieweeは自分の実装に自信が持てるので双方にとって良い
• Revieweeからの質問には誠意を持って答える
  • 指摘内容がうまく伝わらないこともあるので、サンプルコードを用意したり、参考になりそうな記事のリンクを共有したりして、わかりやすく答える
  • コードで対話することも重要
• PR上での議論が難航しそうな場合はMTGやペアプロ、モブプロを提案するのもアリかも
• 自分の好みを押し付け過ぎない
  • 好みの問題なら基本的にRevieweeのやり方を尊重してあげるのが良い
    • 「なぜそちらの方が良いのか？」と問われても好みの問題としか答えようがないと思うし、それでは納得感も生まれない
    • コーディングスタイルに関することなら、Lintのルールやチーム内のコーディング規約を見直せないか提案するのが良い
  • FYIやIMO、nitsなどのタグをつけてシェアするぐらいにしておくのが無難
• 良いと思った点は積極的に褒める
  • レビューは指摘がメインになることが多いので、良いと思った点やレビューを通して何かを学べた場合は積極的にそれを伝えてあげられると良い
  • 褒められて気分を害する人は基本いない
  • こういう文化があるとチームの雰囲気も良くなることが多い (気がしている)

Reviewee編

レビューされる時に気をつけたいポイント

• PRで何をレビューしてもらいたいか明記しているか
  • Acceptance criteria
  • スコープは明確になっているか
    • PR内で何を実装したか
    • PR内で何を実装していないか
      • TODOを残している場合は該当の別Issueを作成しているか
• コードは該当IssueのAcceptance criteriaを満たしているか
  • レビュー依頼前に必ずセルフレビューして、Reviewerになるべく余計な確認をさせないようにする
    • 例えば、レビューしてもらった後に必要なコミットが漏れていたことが判明して、余計な再レビューが発生するなど、ケアレスミスはなるべく未然に防ぐ
• PRのボリュームは適切か
  • クソデカPRはReviewerもレビュー時に問題に気づくことが難しくなるのでなるべく避ける
  • PRを分割してレビュー依頼できないか
    • 例えばライブラリのアップデートを含んでPRが大きくなっている場合、ライブラリアップデートのPRと実際のIssueを解決するPRに分割できそう、など
• 実装は不必要に複雑になっていないか
  • 自分の書いたコードは数ヶ月後、自分も含めて誰が読んでも意図が理解できそうか
• コードだけでは意図が伝わりにくそうな部分はコードやPRに補足コメントを残せているか。別ドキュメントに背景や前提情報を言語化しているか

マインド面

• 自分の実装に関するどんな質問にも答えられるようにする
  • Reviewerからの質問には「なぜそのようにしたか」理由を明確にして回答する
    • 「よくわからないけど、とりあえず動きました」みたいなのは潜在的なリスクを含んでいて基本的にヤバいので、そういう状態なのであればペアプロなども検討して解決しておく
    • 深く考えられていない部分がある場合は「何が分かっていて、何が分かっていないか」を具体的にReviewerもしくは他メンバーに伝えて相談する (レビュー出す前に解決できているのが望ましい気はする)
    • 何かを参照した場合はその参照リンクも共有する
      • なるべく信頼できる、オフィシャルなドキュメントを参照する
• Reviewerの指摘が理解できない場合は積極的に質問する
  • Reviewerの指摘をよく理解できないまま従っていると、納得感も生まれないし、チーム内で知識をシェアするというレビューの目的からも逸れてしまう
  • テキストだけでわからないなら、オンラインやオフラインでMTGすればええやん
• Reviewerからの指摘は自分にとって有益なものであることを理解する
  • 指摘されるとやはり傷つく時もありますよね。分かります
  • レビューでの指摘は他者の良い考え方を学べたり、自分の間違いに気づけたり、成長できる絶好の機会です
  • へこたれない強い気持ちも磨いていきましょう