レビュアーとしてコードを読んできて、レビュー前に確認して欲しいと感じたことについてまとめました。かなり初歩的な話です。
基本的に読み手に立って考える、可読性・保守性意識という内容です。
目次
実装面
エディタの警告が出ていないか
- スペルミスなどを弾ける
- 不要なimportを弾ける
- 不要な変数・引数・関数を弾ける
変数・関数・クラス名などが適切な命名か
- 意味・役割が分からないと修正が困難になる
- 可読性・修正容易性に大きく関わる
コードの統一感
- 統一感がないと可読性が低い
- コードは人が読むもので、読みにくいと読む側も時間を取られる
デバッグコードをそのままにしていないか
無駄に変数/メソッドのスコープが広くないか
- むやみにstaticにしたりしてスコープを広くすると、予期しない箇所で変更される可能性がある
- スコープを最低限にしないと、実装者が予期しない使われ方をされて不具合が起こる可能性がある
- 実装ミスを未然に防げる
アクセサは最低限か
- アクセサが最低限じゃないと予期しない箇所で呼ばれたり変更されて、不具合につながる可能性がある
- 実装ミスを未然に防げる
設計面
クラス・コンポーネントの依存関係・責務は明確か
- むやみに依存させると修正した時の影響が大きくなる
- 責務が明確じゃないと、修正時の影響が分かりにくくなる
- 処理が自然な配置かどうかはコードを読む際に影響を与える
むやみにインターフェース、親クラスをいじってないか
- 既存への影響が大きいため、他メンバーに確認するべき
- 意味があってその作りになっていることが多いので、修正は慎重にするべき
既存アーキテクチャにあった実装か
- MVCやMVPなどアーキテクチャにあった実装方針であること
- 守らないと統一感・レイヤー/責務分割に影響が出る
品質面
(テストコードがある場合)テストが通っているか
- チームによってはCI\CDで自動テストが走るようにしているのでそこで通らないことが分かるかもしれません
自分でしっかり動作確認したか
- 自分でしっかり動作確認してからじゃないと、レビュアー・テスター側の負担になる
- 不具合発見 -> 修正 -> レビューに時間がかかるため、なるべく未然に防ぐようにする
異常系・準正常系が考慮できているか
- 正常系のみで、オフライン時だったりデータがなかった場合の動作が抜けていないか
既存への影響確認
- 修正した範囲は問題ないが、他処理の動作に問題がないか
- 自動テストがあるなら修正箇所以外でテストが通るか
基本的なセキュリティ面の問題がないか
- コマンド/SQLインジェクションがないように生SQLを使っていないか
- Web/モバイルなら、フロント側だけじゃなくてサーバー側でもチェック処理をしているか
仕様通りの動作・レイアウトか
その他基本的なこと
- デバッグコードが残っていないか
- ネストが深くないか
- 処理が冗長じゃないか、共通化できないか