■ 1. 対象読者と記事の目的
- 対象読者はコードレビューする側に回ったばかりの1〜2年目の新人エンジニア
- リードエンジニアがチーム内でレビュー手順や観点を議論する際の叩き台としても活用可能
- 記事の目的はコードレビューの観点を全網羅することではなく「先輩がレビュー時に何を見ているか」という気づきの提供
- 可読性・保守性・堅牢性・正しさ・セキュリティ・パフォーマンスの6テーマに分けて各3点ずつ観点を紹介
■ 2. 可読性の観点
- シンプルさ:
- 標準ライブラリで代替できる自前実装は書き換えを検討する
- 短縮しすぎて可読性を損なうのは不可
- 命名:
- わかりにくい変数名やメソッド名はNG
- ほどよく具体的でほどよく抽象的な名前を付ける
- 動詞と名詞の組み合わせ方で意味が変わる点に注意（例:
upload_filesvsuploaded_files）- 1行の長さ:
- 人間がぱっと理解できない長さの1行記述はNG
- 一度変数に代入してから使用する方が理解しやすい
■ 3. 保守性の観点
- 責務の分離:
- ロジックをコントローラに書くとfatコントローラ問題が発生し再利用性が低下する
- ビジネスロジックはモデルに委譲することでコントローラを薄く保てる
- カプセル化:
- オブジェクト外部から内部状態を直接参照・変更するのはNG
- 判断や操作はオブジェクト自身に任せることで再利用性と可読性が向上する
- DRY原則:
- 同一ロジックの繰り返しを発見した場合はDRY化を検討する
- 将来的に別々の変更が入る可能性がある場合はあえてDRYにしないケースもある
■ 4. 堅牢性の観点
- 配列とハッシュの使い分け:
- 無関係なデータを配列に格納してインデックスで指定すると要素の増減時に不具合が起きる
- ハッシュはキーで指定するため要素が増減しても既存コードの変更が不要
- ハッシュのキーtypo問題:
- ハッシュはキーをtypoしてもnilが返るだけでエラーにならない
- 一定の複雑さを超えたら専用クラスを定義しメソッドで参照することでtypoを検出可能
- 文字列結合によるフォーマット生成:
- URL・HTML・XML・JSON・SQL・CSVなどを文字列結合で生成すると予想外の値でフォーマットが破綻する
- 各フォーマットには専用のライブラリを使用する
■ 5. 正しさの観点
- ゼロ除算:
- 除数を動的に変更する場合はゼロ除算でエラーが発生する恐れがあるため考慮が必要
- 丸め誤差:
- 小数を含む計算ではFloat型特有の丸め誤差が発生する可能性がある
- 金額計算では致命的な問題になるため RubyではBigDecimalを使用する
- 要件の充足:
- 実装がPRのdescriptionに記載された要件を満たしているかを確認する
- 正常系だけでなく異常系（例: 決済処理の途中キャンセル）も考慮できているか確認する
■ 6. セキュリティの観点
- 権限チェック:
- フロントエンドでボタン表示を制御するだけでは不十分
- Webアプリケーションではクライアントが自由にリクエストを送れるためバックエンドでも必ず権限チェックが必要
- XSS・SQLインジェクション対策:
- フレームワークやライブラリを素直に使う限りほとんど発生しない
- SQLを文字列結合で動的構築するなどイレギュラーな実装を行う際は十分な注意が必要
- クレデンシャル管理:
- APIキーなどをコードに直書き（平文でgit管理）するのはNG
- 環境変数やフレームワークが提供する仕組み（Railsの場合
config/credentials.yml.encなど）を使用する■ 7. パフォーマンスの観点
- 本番データ量の想定:
- ローカルで一瞬で動く処理も本番のデータ量では異常に時間がかかる場合がある
- 単純なデータ更新はプログラミング言語側でループするよりDBのSQL一発で処理する方が高速
- 計算量の意識:
- ループ内で毎回最大長などを求めると計算量がO(n^2)になり要素数増加時に無駄な処理が急増する
- 最大長などはループの外で事前に求めておくことで計算量をO(n)に抑えられる
- DBアクセス回数:
- DBへのアクセスは基本的に遅く N+1問題や同一データの繰り返し問い合わせは本番環境の負荷に耐えられなくなる恐れがある
- シンプルなループ処理に見えてもその裏側でDBへの問い合わせが大量に発生している場合があるため注意が必要
■ 8. その他の観点
- 言語・フレームワークの考え方やベストプラクティスに沿った「○○らしいコード」を書いているか
- プロジェクト固有のお作法に従ったコードを書いているか（お作法は暗黙知にせず明文化することが望ましい）
- 成功・失敗を表すメソッドの戻り値を無視していないか
- 例外処理が適切か（エラーを握りつぶしていないか/不要な例外キャッチをしていないか/条件分岐で実装できるものを例外でスロー・キャッチしていないかなど）
- DBトランザクションを適切に利用しているか
- アプリ側の変更に対応したテストコードが追加されているか
- テスト設計手法（境界値分析など）を意識したテストコードになっているか
- 複雑な条件分岐を持つロジックに対するテストコードのカバレッジが十分か
- false negativeなテストコード（バグが発生していても検知できないテストコード）を書いていないか
■ 9. コラム：「5年後に自分がメンテできるか？」という視点
- 業務コードの寿命は非常に長くPR作成者が退職した後に自分がメンテすることも現実として起こりうる
- レビュー時の自問事項:
- 5年後にこのコードを見てロジックをすぐに把握できるか
- テストコードがない状態で既存機能を壊さずに自信を持って変更・追加できるか
- 当時の仕様の背景を5年後にすぐ振り返られるか
- 答えがNOの場合は「わかりやすいメソッド名への変更」「テストコードの追加」「PRのdescriptionへの詳細記載」などのレビューコメントを入れる