トップ > コードレビュー時の心構え

コードレビュー時の心構え

社内の勉強会で「コードレビューのコツ」というタイトルで、3人で分担して発表しました。 3人が被らないよう、自分は抽象度高めで心構えについて話しました。

当日入っていた実コードのスクショは入れられないのでだいぶ分かりにくい気がしますが、せっかくなので残しておきます。

レビュー時の心構え 1

大から小へ

  • 細かい指摘で満足しないように気をつける
    • 無意識にdiffを見ると、細かい指摘をたくさんして満足しかねない
  • 大枠から把握していく
    • レビューの目的は何か理解する
      • どの段階でのレビュー依頼か把握する
        • 仮実装なのか、正常系は完了なのか、テスト等も完了なのか
    • 変更の目的を把握する
      • 要件定義書や基本設計書を読む
      • 暫定対応や納期優先なのか、将来を見越して結構変えているのか
    • 規模感や、肝がどこか把握する
      • Pull Request の Files changed をざっと下まで見る
      • 触っているファイル数や、影響範囲を把握する
      • diff ごとに、ざっくりリファクタの部分と機能（仕様）変更の部分を把握する
    • 肝となるところを確認する
    • ここまで来て初めて、上から丁寧に見る
    • 必要ならgit pullして慣れているIDEで見る・動かしてみる
      • 呼び出し元ジャンプしたり検索しながら見る

レビュー時の心構え 2

具体的にイメージする

  • リリースをイメージする
    • 単体でリリースするのか、他とセットなのか
    • リリース時のアクセスどうなる？
      • カラム追加、既存データのデフォルト、など
        • 「 migration流れた」〜「コードのリリースが 終わる間」のアクセスもエラーは出ない？
      • ALTER TABLE の時間どのくらいかかる？
        • テーブルロックされない？
      • 「ユーザーが画面を開いた」 →「リリースされた」→「ユーザーが登録ボタンを押した」ってなっても大丈夫？
  • 運用をイメージする
    • ログ出す必要ない？
    • ログ見て、ちゃんと処理されてるか分かる？
      • 後で調査できる？
    • エラーが起きてもデータ不整合にならない？
      • データはロールバックされる？不整合にならない？
      • 特に繰り返し処理の場合
        • どこまで進んでいるか分かる？
        • エラーが出たら止まるべき？スキップして次に行くべき？
        • エラーが出ても進める場合は、スキップしたことを検知できる？
    • 途中から再開できる？
      • IDとか指定して部分的に再実行できる必要とかはない？
      • 冪等？(主にバッチ処理系)

レビュー時の心構え 3

未来を考える

  • 対象データやDBのレコードは、どのくらいのスピードで増えるか
    • データ量や処理速度は何かに比例する？
      • O(n*2) になってない？
    • n年後には何レコードくらいになる？
      • その Select、データが増えても1回で大丈夫？メモリに載る？
  • 後で読んで分かるか
    • 読んでも分からない意図や背景がないか
      • コメントに残っているか
    • Pull Requestも、後で見られても分かるようにする
    • 直接話したこともメモを残す
      • 「口頭で話したとおり○○○が良いと思います」
      • 「○○○ですが、○○○と話したので今回はこのままとします」
      • Slackのリンクを貼っておくのも良い

その他のコツ

先回りする・具体的に・効率的に

  • 依頼時は、概要や関連資料、見てほしい観点等を伝える
    • 絶対聞かれるだろうなと思うところは先にコメントしておく
      • Pull Request 作ったら、自分で diff にコメントしても良い
  • レビューがおわったら
    • 何を見たか・見ていないか伝える
      • 「○○までレビューしました。○○の部分を直したら再レビューします」
      • 「このへんは自信がないから◯◯さんにも見てもらってください」
  • 具体で話す
    • コードで伝える
      • 「○○のほうが○○になるのでいいと思います。<具体的なコード...>」
      • 「○○メソッドを使うと簡潔に書けます。<具体的なコード...>] 」
    • 根拠を載せる・URLを載せる
    • 図で伝える
      • 簡単な手書きの図でも、あるとぜんぜん違うこともある
  • 必要なら対面で話す
    • 指摘が多いとき
    • 伝わりにくそうだと思ったとき
    • 議論したいとき

質の高い議論のために

勉強する

  • 言語やフレームワーク、ライブラリの機能
  • インフラ構成
  • 計算量とアルゴリズム
  • 速度感覚を持つ
    • メモリに乗せる ＞ 優先ネットワーク（≒データセンター内） ≧ ディスクアクセス ＞ サーバーをまたぐ ＞＞＞ インターネット越し（外部サービス）