コードレビューのイロハをまとめました（レビュイー/レビュアー両者の心得や実践的なテクニックなど）

１．記事の目的

業務でレビューを依頼されることがありますが、どのような観点でレビューをすればよいのか、これは指摘すべき項目なのかと、色々と判断に思い悩む時があります。
そこで、Google's Engineering Practices documentationや Qiita の人気記事を網羅的に引用し、自分のためのガイドラインを作成しました。
ご指摘等あればドシドシお願いします！

• 前知識
  • レビュアー(reviewer)：レビューする人
  • レビュイー(reviewee)：レビュー依頼する人、実装者

２．レビューの目的

そもそもレビューは何の為にするのか。

プログラムには様々なバグが潜んでいる。大雑把に分類すると下記に大別される。

• 自明の問題

  • コーディング規約に従っていない、スペルミス/タイポ、命名、ロジックの誤りなど、目に見えている部分の問題

  • 原因
    - エンジニアとしての経験量 （初級レベル：一般的な目安としては3年未満）
    - 使用言語・フレームワークの理解
    - チーム内での規約の不統一
    - 集中力の問題

• コードに現れない部分の問題

  • プロダクトの細かい仕様の理解

  • エッジケース

  • 並行処理問題

  • 変更しようと思っていない部分に対する副作用の考慮

    • 同じテーブル・コンポーネント・クラスを使っているなど

  • 将来的に生じるであろう問題の予測

  • 原因
    - エンジニアとしての経験量（中級〜上級レベル）
    - プロダクトの仕様に関する深い理解

  • **　しかし経験が浅くとも、コードに現れない部分を意識することで一歩踏み込んだレビューが出来るようになる**

• 具体策・マインド

  • 自分が実装するならどこを気をつけるか当たりをつけて読む
  • コードの不吉な匂いなど典型的な悪いパターンをある程度知っておく（「5.レビューチェック項目」を参照）

（余談）初級者によるレビュー

• 初級者にとって、コードをたくさん読むことは技術力の向上に良い影響がある
• 初級者の方が丁寧にレビューするため、ベテランエンジニアの負荷が軽減するという話も

３．レビュイーの心得

レビュー依頼粒度

• できる限り小さい粒度で依頼する
  • 未完成状態ではなく、自己完結型（それ自体で成り立っている状態）の変更であることが望ましい
• 変更の数が少ないと発生するバグも少なくなる
  • 差分が多すぎると、レビュワーの負担が大きくなり、細かいところまで見切れなくなる可能性がある
• １つの機能追加のチケットでも、画面毎や区切りのつく単位毎にする

レビュー依頼前にリファクタリングする

• 余分な変数、ロジックがないか
• コピペしたコードの場合に余分なコードが含まれていないか
• 明らかな文法ミスがないか
• 具体策：
  • Linter（静的解析ツール） を使用する
  • 高性能な IDE を利用し、文法ミスを事前に潰す

テストを書く

• リリースしたコードにバグがあった時、レビュアーにも責任はあるが、基本的にはレビュイーの責任
• レビュアーに確認してもらいたい箇所は積極的にテストを書く（レビュアーもテストのレビューの方がしやすい）
• 特にモデルに相当する部分の単体テスト、サービスのコアとなるような重要な機能のテストは必ず書く

何のためのプルリクか伝える

• どのような変更を加えたのか、何故行ったのかについて明確に伝えることによって、レビュワーが考える負担を減らす
• 望ましい構成
  • 1点目：シンプルなタイトル
    • 例）「機能変更」、「リファクタリング」など
  • 2点目：対応内容に関する明快で実用性の高い要約
    • 悪い例）「バグの修正」、「便利な機能を追加」
    • 良い例）「rpc：RPCサーバーメッセージフリーリストのサイズ制限を削除」「TimeKeeperでタスクを作成し、TimeStrメソッドとNowメソッドを使用」
  • 3点目：詳細な解説
    • 解決した問題の簡単な説明や、これが最善のアプローチである理由など
    • アプローチに欠点がある場合は、それらについて言及する
    • 外部リソースへのリンクを含める場合は、将来の読者にはリンク切れで表示されない可能性があることを考慮し、きちんと文章で残す
  • 参照 - 適切なCL記述の作成 - Google's Engineering Practices documentation
• 対応する Issue がある場合はそれを明記
• プルリクテンプレートを活用するとチーム的に平和
  • Pull Request のテンプレートを作って効率よくレビューしよう！

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

• レビュイーに対する人格攻撃ではないことを理解する
  • レビュアーも人間であり、気分の上がり下がりが当然あって、それがコメントに現れるときもある
  • レビュイーはそれに対して備える必要があり、そのようなコメントに遭遇したときは「レビュアーが私に伝えようとしている建設的な内容は何か？」と自問する
  • 建設的でないコメントに心をが乱された場合は、一旦落ち着くまで他の作業をするかPCから離れる
  • 必要であらば直接レビュイーとコンタクトを取り、丁寧に話し合う。そこでも建設的な対応が得られない場合はマネージャーに相談をすることも検討する
• コードを修正する
  • レビュー担当者がコード内の何かを理解していない場合、まずコード自体を明確にする必要がある
  • 一人のレビュアーが特定のコードの理解に困ったときは、将来の読者も理解できない可能性があるため、別の書き方がないか、コメントによって補足説明できないか検討をする
• 自己成長
  • 一般的にレビュアーは、何が最善かについてレビュイー自身で考えて欲しいと思っている
    • 大抵の場合、レビュイーのほうが修正内容のコードに長く触れているため、レビュイーのほうが最善策・回避策が思いつきやすい
  • レビュアーを納得させるということは、エンジニアとしても社会人としても大きな成長を得ることが出来る

４．レビュアーの心得

レビュー依頼への応答速度

• コードレビューの返信は、最長で一営業日（出来る限り翌朝一番に返信すべき）

  • 個人ではなく、チーム全体の開発速度が向上させるため
  • レビューに素早く反応しなくてもレビュイーは他の仕事を終わらせられるが、レビュー待ち・再レビュー待ちとなると何日も遅れることになる
  • また、 開発者がコードレビューのプロセスに不満を持ち始める
  • レビューの遅れはコードを綺麗にしたり、リファクタリングしたり、既存の変更を更に改善する意欲をそぐ
  • レビュアーのレビュイーに対する不満は、大抵の場合、レビュースピードを向上させることで解消されることが多い

• ただし、コードディングのような集中力を要するタスクを中断してまでコードレビューしてはいけない

  • 開発者が割り込み作業のあとでスムーズな開発フローに戻るのには長い時間を要する
  • 仕事のブレークポイントを待ってからレビューのリクエストに返信する
  • おすすめのレビュータイミングは、ランチ後やMTG終わりやコーヒーをお代わりした際など

• レビュアーが非常に忙しくて、レビュー依頼をされても十分な時間が取れない場合

  • もっと短時間で応答できる他のレビュアーを紹介
  • もしくは広い観点で見た初期段階のコメントを残して返信

• 量が多くて短時間では返信できない場合

  • 巨大なプルリクではなく、それぞれが関連している小さなプルリクに分割するようレビュアーに依頼する
  • レビュアーにしてみれば仕事が増えるが、普通のプルリクでは不可能な作業ではなく、レビュアーにとっては非常に助かる

レビューの基準

• 「完璧な」コードを求めるのではなく、「システムを確実に良くする」コードを求めるということ
  • レビュアーは、全ての細かな部分を磨くように要求するべきではない
  • 提案している変更の重要性と比較して、前進する必要性のバランスをとるべきである
  • レビュアーが求めるべきは、完璧ではなく、継続的な改善
  • もちろんレビュアーが求めない機能を実装している場合は、いくらコードが完璧でも拒否して良い
• 「個人的な好み」は排除する
  • 文法（スタイル、規約）はチーム内のルールや言語仕様を絶対とし、なければチーム内で規約を作成する
  • もしくはGoogle Langage Styleguideを参照する（メジャー言語からマイナー言語まで色々公開されている）

コードレビューの手順

• 変更を広く眺める
  • 何のための変更なのかを確認
  • 行ってはならない変更であれば、変更すべきでない理由を添えてすぐに返信
• 「最悪を最初に」
  • 誤りがプロダクト全体に影響し、手戻りのコストが高くつく、あるいは失敗するようなリスクがないかを確認する
    • 基本的な初期フェーズの要求仕様
    • クリティカルな決定の基礎になる仕様
    • 使用頻度が高いモジュール
    • バグや設計変更があった時に大変になる箇所
    • サービスの根幹に関わる箇所
• 残りを適切な順序で見る
  • 設計上の重大な問題がないことが確認できたら、残りを適切な順序で確認
  • 主要なファイルを確認し終えたら、大抵の場合はコードレビューツールが表示してくれる順序で各ファイルを調べるのが良い

コメントの仕方

• 礼儀正しく丁重に
  • 明確で有益な内容であることもちろん大切だが、礼儀正しく敬意を払った書き方であることも大切
  • 必ずコードについてコメントし、レビュイー本人についてコメントしないよう心がける
  • 例）「あなたは何故この並行処理をしましたか？」⇛「この並行処理はこういう場合に問題になる可能性が高いですが大丈夫ですか？」
  • 相手を傷つけたり怒らせたりするかもしれない繊細な内容を書くときには必ず意識する
• 理由を明確に説明する
  • コメントの「理由」をレビュアーが理解できるように書く
  • コメントの意図についてもう少し詳しく説明したり、参考になるベストプラクティスを提示したり、その提案がどのようにコードの質を良くするのかを解説する
• 指摘と指示のバランス
  • レビュイーのほうがコードをじっくり見ているはずなので、レビュイーのほうが良いソリューションを持っている場合が多い
  • 問題を指摘して決定を開発者本人に任せることは、レビュアーの学ぶ機会になり、加えて長期的にコードレビューが楽になる
  • ただし、直接的な指示や提案が必要なフェーズもある

念入りに見る項目

• メール送信
• セキュリティ
• 請求、決済関連
• ユーザー入力を記述している場所

なお、セキュリティ・並行処理・i18nなど、コードが理解できても自分にレビューの資格がないと感じたときは適任者に依頼すれば良い

レビュー粒度

• 1回のレビュー量は、1時間かつ400～500行 （あくまで目安）
  • 脳は大量の情報を一度に処理することができるが、大容量のコードを長時間に渡ってレビューすると欠陥検出率は下がる傾向にある
  • 集中を要する仕事を行っていると、約1時間後にはパフォーマンスが下がってくることが知られている
  • 一定期間ごとに休憩を入れることで、仕事の質が大幅に向上する
• コードが読みにくいことでレビューに時間がかかっているのなら、レビューをいったん置いてレビュイーに知らせ、コードを分かりやすくしてくれるのを待つ
  • レビュアーがコードを理解できないのなら、他の開発者もきっと理解できない
  • 開発者にコードを分かりやすくするよう依頼することは、未来の開発者がそのコードを理解する手助けとなる

５．レビューチェック項目

機能

• 最重要

  • 機能要件を過不足なく満たし、正しく動作しているか
  • 仕様が明確ではない部分について、想像で実装してないか
  • 現在のシステムにとってまだ必要のない機能を盛り込んでいないか
    • 開発者は現在解決する必要のある既知の問題に取り組むべきである
    • 将来解決する必要が出てくるかもしれない推測に基づいた問題は、それが発生してから取り組めばよい

• 次点で優先度が高い項目

  • 複数端末で同時に実行した際に、不具合は発生しないか
  • 運用時に問題が発生しないか
  • 不正アクセス対応はされているか
  • 重要な部分はログ出力されているか
  • 課金周りの処理の計算は正しいか
  • 集計処理について GoogleBot などは弾いているか
  • 処理速度が極端に遅くないか

コード

• コーディング規約に準拠しているか
• 一時的に使用した不要なコードが残ってないか
  • dump や過度な log など
• 不必要に public がつけられていないか
  • まずは private で作り、必要なら protected, public と広げていくのが適切
• xxxUtil, xxxHelper, xxxManager などの神クラスが作られていないか
  • 役割がはっきりしない名称のクラスは危険
  • 作った時点では良いが、5 年 10 年とメンテナンスされていくうちに機能を押し付けられ追加されて手がつけられなくなっていく
  • まずは Factory, Builder, Writer, Reader など、役割が限定されるような名称に変えられないかを検討する
• 可読性の良いコードかどうか
  • 処理を関数化する
  • コード行数 20 ～ 30 行以内推奨（長い関数は分解化する）
  • 意味のあるコードの塊で改行を入れる
• ハードコーディングしてないか
• コードの中に直接書くべきでないリソースが含まれてしまっていないかどうか
• 機密性の高い情報：パスワードやフォルダ名
• 将来変更される可能性のあるデータ：税率や祝日の日程
• マジックナンバー：ステータス値や税率、フラグ値など
  ⇛ 対応：マジックナンバーは定数化
• 全角文字がベタ書きされていないか
  • 全角文字がコード内にベタ書きされていると、多言語対応時にコードを修正する必要が発生する
  • 大半のフレームワークは i18n 対応方法が用意されているためそれを使用する
  • 将来に渡って国際化が必要がないと判断されている場合はスルーして良い
• 参考：リーダブルコード

ロジック

• 同一のロジックが 2 箇所以上で書かれていないか
  • DRY（Don't repeat yourself）原則を遵守
• 意味の無いロジックが含まれていないか
• デッドコード（絶対実行されないコード）がないか
• 冗長な条件分岐を書いていないか
  • 早期 return、continue
• ネストが深くなりすぎていないか(2~3 重まで)
• ループでは 0 回、1 回、複数回のパターンを想定しているか
• 適切な例外処理が入っているか
• 循環的複雑度が 10 ～ 20 かどうか
  • 循環的複雑度とはプログラムの複雑度を測る数値
  • 高性能な IDE、Lint 等の外部プラグインを導入することで算出できる
  • 10 以下推奨
• 取得したメインデータをループさせて処理している場合、取得するのが大量データになったとしても速度的に問題がないか
• boolean 変数の否定分岐がないか
  • 結局 if 文内はどっちなのか本人ですら混乱し、不具合を仕込む可能性が上がる
• 関数引数の存在チェック(undefined, null, nil)
• 境界条件でうまく動くか
• 計算オーダーが短く書けないか、DB 検索クエリ条件で解決できないか
• 並列処理は同期や整合性が取れているか
• キャッシュを利用している箇所の場合、キャッシュ有無で両方問題ないか
• オブジェクト参照してる変数を上書きして意図しない挙動になっていないか
  • 必要に応じてディープコピーする

命名など

• タイポ、スペルミス、表記ゆれがないか
• 命名規則を統一しているか
• 変数名から内容が予測できる名称になっているか
  • 長さはあまり気にしないが、安易に長い名前はつけない
• メソッド名が適切か
  • 機能を明確に表した名前になっているか
  • メソッド名から返ってくる値が予測できるか
  • getXXXMessage()の返り値で integer が返ってきていたりしないか
  • メソッド名から想定しづらいような副作用が存在しないか、もしくはドキュメント化されているか
  • get メソッド中で set をしていないかなど
• boolean の変数名に xxxflag を使っていないか
  • xxxFlag だと、true の時にどうなるのかが分かり辛い。
  • writeFlag ではなく、writable が適切
• 他のモジュールで使われている名前と別の名前になっていないか

コメント、Doc

• 「what」ではなく「why」がコメントされているか
• 第三者が見た時に理解できるコメントになっているか
• コメントが処理とイコールか
• 処理が複雑な場合、仕様の説明がコメントで書かれているか
• 特殊なロジックが入っている場合には、何故そうしたのかについてコメントで書かれているか
  • 複雑な処理の場合はコードブロックでの要約は書いた方がよい
  • 難解な処理や複雑な条件分岐
  • コードから読み取れない特殊な経緯や意図
• 自明な情報をコメントとして書いていないか
  • コードをただ日本語訳したようなコメントを一行一行書いてしまうと可読性が悪くなる
• やむを得ずマジックナンバーを使う場合に、コメントで意味・意図が書かれているか
• 関数が何を Input とし、何を Output とするのかについて適切に書かれているか
  • Array や Json Object を使っていて返す構造が不明瞭な場合には、コメントにどのような形式で返ってくるのか明記する
• コードに何か使用上の注意点がある場合にコメントで注記されているか
• 後で対応する系のコメントには、TODO:、FIXME:キーワードが付いているか
  • TODO：後で見直す場合
  • FIXME：後で必ず修正する場合
  • HACK:リファクタが必要なキレイじゃないコード
  • ...etc
  • 参考：TODO: 以外のアノテーションコメントをまとめた
• 過去のコードがコメントアウトされて残っていないか
  • 基本的には git の履歴を頼る
  • 重要な経緯があり残すのであれば、経緯の説明をコメントする

SQL

• 指定されているデータ型が適切か

• 変なデータが混入するケースがないか（モデルでvalidate しているか）

• SQL 実行数が多くないか

  • 1 つ 1 つの SQL は速くても、積もるとレスポンスは遅くなり、DB サーバの負荷になる
  • insert の場合、bulk insert できないか（bulk insert：CSV 形式などの形式で、テーブルに一括でデータを登録できるコマンド）
    • 100 回 insert 文を実行するよりも 100 レコード文の bulk insert の方が速い
  • select の場合はまとめ取りできないか
  • 外部リソースへのアクセスはまとめて実行しているか

• 実行 SQL に index が使われているか

  • index 無しでデータを取得していると、データ量と共に劇的に遅くなる
  • 複合 index が使用される場合、where 全列に index が使われているか
  • order by 列にも index が効いているか
    • ソートする列に index が効いていない場合、filesort が発生するので遅くなる
  • 使われていない列に index が貼られていないか（不要な index が無いか）
  • カーディナリティが低い列に index を貼っていないか
    • カーディナリティが低い（重複データが多い）列は index 効率が悪いので遅くなる
  • 必要ない列を取得しているためにカバリング index が失われていないか
    • カバリング index：SQL でデータを取得する時に必要なデータを全て含んでいる index
    • SELECT *で取得するとパフォーマンスが悪く、必要な行・必要な列のみを取得する方が良い
    • MySQL はクラスタ index のため、select 列が index でまかなわれていれば速くなる

• レプリケーションを用意している場合 slave 遅延が考慮されているか

  • slave 遅延：DB のレプリケーションが非同期で行われるために、スレーブ DB の更新がマスター DB の更新より一瞬遅くれてしまう現象

• トランザクションが使えるシステムならトランザクションを使用しているか

• 複数のリソースを同時に変更する場合にはロックをかけているか

• 複雑な SQL の場合、explain して key_len が適切かチェックしてあるか

• 整合性が重要なテーブルの場合、unique key が貼られているか

  • コードの不具合によりデータの整合性が崩れるのを阻止する

• select for update を使用している場合、ギャップロックを考慮しているか

  • 存在しないデータに select for update をかけていると、ギャップロックによるデッドロックが発生する可能性がある
  • select for update
    • select 文の発行と同時に行ロック（レコードロック）をかけること
    • select 文で取得したレコードに update をかけたいときなどに用いる
  • ギャップロック
    • レコードとレコードの間への挿入を停止するロック
    • 結果の一貫性を保証する

• テーブルの想定レコード数が多すぎないか

  • データ数が多いと取得速度も ALTER TABLE 実行速度も遅くなる

• パーティションやシャーディングなどの対策がとられているか

  • パーティション
    • DBのテーブルを物理的に分割すること
    • 絞り込み条件がパーティションの分割条件と一致する場合、条件に合わないパーティションは早期の段階で処理対象から外され、無駄な検索が行われず性能が向上する
    • 例えば月単位で分割している場合での、月ごとの集計処理などで有効
  • シャーディング
    • データを複数のノードのディスクに分割配置すること
    • DB をユーザーの ID でシャーディングしている場合、DB を増やすことで更新負荷を複数のサーバーに分散することができるため、アクティブユーザーの増減による DB 性能の拡張性が高い

• DB クエリエラーになるケースがないか（クエリによってエラーになるケースがないか網羅できているか）

• DB フィールドの変更・削除に伴う修正が必要な場合、実データを localhost の DB に落としてきてマイグレーションを確認しているか

  • 実データは意図しないデータが入っていることが多いので、きちんと動作するか確認する

• マイグレーションが発生する場合、フォールバックの処理が書かれているか

トランザクション編

• トランザクションの処理はデッドロックが発生しないか
• トランザクション範囲は適切か
  • コミット & ロールバックするべき範囲で括られているか
  • 範囲が間違っている場合、例外が発生したらデータの整合性が保てなくなる
• DB が分かれている場合、XA トランザクションを使用しているか
  • XA トランザクション（分散トランザクション）：複数のDBにまたがるトランザクション処理
• 同リクエスト内で、MySQL と Redis 更新が混在する場合、例外が発生した時のことを考慮しているか
  • Redis を先に更新して DB 更新時に例外が発生しても、Redis はロールバックされないため DB との整合性が崩れるので注意

画面、フロント

• 利用者視点(特に誰が、どのように使うか)で適切な文言・動作になっているか

• 操作する上で足りてない情報、多すぎる情報がないか

• 視覚情報として見やすいかどうか(違和感のある表示ではないか)

• ユーザが操作不能になるフロー・導線はないか

• セキュリティ的に問題のあるパラメータが渡ってこないか（XSS、SQL インジェクションなど）

• ブラウザの開発者ツールでJavaScript エラーが出ていないか

• リンク切れがないか、リンクが正しいか

  • メール、メッセージ送信系（SMS、LINE）は実際に送信し、開封したメッセージでもリンク切れがないか

• エラー時のフロントエンドの表示は問題ないか

• ブラウザ依存の実装はないか、マルチブラウザ対策できているか

  • Chrome, iPhone Safari, IE など
  • プロパティはCan I use でまとめて確認するのがおすすめ

• SPA の場合、他のルーティングを上書きしていないか

  • SPA(Single Page Application)：ブラウザによるページ遷移を行うことなく、単一の Web ページでコンテンツの切り替えを行うことで UX 体験が向上する Web アプリケーション

• 不正なユーザ入力で問題が発生しないか、FatalError にならないか

  • ユーザ入力データを表示する箇所は XSS のサニタイズができてるか
    • サニタイズ（Sanitize）：危険なコードやデータを変換または除去して無力化する処理
    • Web サイトに設置された入力フォームなどから悪意のあるコードが入力され、その文字列が実行されることで様々な被害に遭う可能性がある

• データ取得系コンポーネント（リスト等）の場合、データが空の状態の表示（もしくはローディング表示）は実装できているか

• 画面サイズを変更してレイアウトが崩れないか

• 表示されている画像のサイズは適切か

• 余白が揃っているか

• 文字サイズが揃っているか

• カラーコードに準拠しているか

• 入力に対し適切な validation が行なわれているか

API

• パラメータ
  • リクエストパラメータの存在チェック(undefined, null, nil)をしているか
  • リクエストパラメータに意図しないデータが入ってこないか(validate)
  • パラメータが配列受け取りの場合、不正に数を増やされても大丈夫か
    • 配列数に上限を設けないと、悪意のあるユーザから大量データに改ざんされたアタックを受けて、サーバ負荷が上がる可能性がある
  • ありえないパラメータが送信されてきた時の対策はとられているか
    • 悪意あるユーザにより、パラメータを改ざんして API が実行された場合、整合性チェックが行われていないとデータ整合性が崩れる
• パス
  • API パスが間違っていないか（他の API パスを上書きしていないか）
  • CRUD（Create Read Update Delete）に合わせた API パスになっているか
    • Create: POST
    • Read: GET
    • Update: PUT
    • Delete: DELETE
• 同ユーザが同リクエストを複数回連続で送信してきても大丈夫か
  • 本来 1 回であるべきリクエストが 2 回連続で送信されてきた時に排他制御が行われていないとデータの整合性が崩れる
• 複数の API の条件分岐に影響するケースがないか
• API 単体テストが書かれているか
  • 正しいデータが取得できているか
• API エラーの場合の動作が実装出来ているか
  • 外部サービスの API を呼び出す際のエラー処理をしているか

テスト

• テストカバレッジ
  • codecovやcoverallsといったようなツールを使うことで、コードのカバレッジを計測して可視化したりslackに通知したり出来る
  • コードカバレッジを100%に保つ必要はないが、システムのコア機能のカバレッジは、テストの抜け漏れを防ぐために有効
  • ただし仕様漏れが存在する場合は実装側で抜け漏れが発生し、カバレッジが高くてもテストの方でも抜け漏れとなるため注意
• テストの自動化
  • Circle CI、Travis CI、Jenkins などの CI(Continuous Integration)を使うことで git push 時に継続的にテストを実行することができる
  • 回帰テスト
    • ライブラリのバージョンをアップグレードしたり、仕様変更した際に他の箇所と不整合（デグレ）がないか検出できる
• 単体テスト
  • 正常系のテストが書かれているか
  • 容易に起こり得る異常系のテストが書かれているか
  • Jest（NodeJS の場合、これに sinon、supertest、nock、rewire、proxyquire などのモックライブラリを組み合わせる）を使ったりする
• 結合テスト
  • テストシナリオとテストケースが適切か
  • テストシナリオ：一連の処理（業務）を最初から最後まで通したもの
  • テストケース：1つのシナリオで確認するポイント
• E2E テスト
  • 単体テストとは違いシステム全体を実稼働時に近い状況で動かしつつテストする手法
  • Webサービスの場合は、ユーザと同じようにブラウザを操作し、挙動が期待通りになっているか確認する
  • puppeteerなどのブラウザ操作を自動化するツールを使ったりする
• ビジュアルリグレッションテスト
  • 視覚的な回帰テストという意味で、スクリーンショット等を撮影しておき前後で差分が出ていないか検証するテスト
  • storybook（コンポーネントのカタログ）とBackstopJS（ビジュアルリグレッションテストができるnpm パッケージ）を利用したりする

Redis

• 使用メモリサイズは問題ないか

  • 使用メモリがmaxmemory を超えてしまった場合、エラーになったりeviction発生によるHIT率が低下する
  • 用意するサーバと較べてメモリ使用量が大きすぎる場合、Hash型でまとめられるか

• expire（生存時間、TTL、Time To Live）が設定されているか

  • expireが設定されていないと、オーバーするまでメモリが肥大化する

• 複数コマンドを実行している場合、パイプライニングを使用しているか

• keys * コマンドを使っていないか

  • ワイルドカード探索は、データの偏りがあるとI/O 待ちでサーバが死ぬ
  • keys コマンドで線形探索するのであればRDBに入れてIndexを張ってWHERE句発行をする

• ロールバックによるデータ不整合に備えた設計になっているか

  • DB トランザクション中に問題が発生し、ロールバックするとき Redis はロールバックされない
  • ロールバックされ RDB と Redis 間でデータ不整合が発生したときに、正しいデータに切り戻される設計になっているか
  • RDBとRedisに重複してデータを持ち、ViewからはRedisにアクセスして高速化の恩恵を受け、データ更新時はRDBの値を正としてRedisデータを上書きする

• Redis のデータが消えたとき復旧できるか

  • Redis が飛んだりデータ不整合が発生したとき復旧できるよう、事前に全復旧コマンドを用意しておく
  • RDBに保存さえ出来ていれば、障害起きてからコード書いても問題ないので必須の対応ではない

• キャッシュとしてではなく、ストレージとして(永続化前提として)使っていないか

  • 基本NGだが、ソート済セット型（SortedSet）などを使う必要があるなどのやむを得ない場合、専用のDB番号を使用する
  • キャッシュとして使っているキーと同一 DB 番号に入っていると、緊急時に削除しにくくなるため
  • AOF設定なり、生成バッチなり、データ永続化や復帰の対応方法が考えられているか

参照

様々な記事を参考にさせていただきました。ありがとうございます！

ソシャゲエンジニアの自分がコードレビュー時に重視する箇所 33 選 【随時追加】
メンバーに恨まれそうな 3 つのコードレビュー施策を徹底したら、逆にメンバーが爆速で成長した話
Java 初心者時代にコードレビューで指摘された悪しき習慣
Redis 本番障害から学んだコードレビューの勘所
コードレビュー虎の巻
コードレビューの際に気をつけること
コードレビューの際によく指摘するポイントについて
【和訳】コードレビューのベストプラクティス