綺麗なコードを書くためのコードレビューチェックリスト
PR出す前にこの観点は必要だよねリストまとめ
▪️ 記事書いた人
1. 設計と仕様の整合性
コードが既存のシステム設計に一致しているか確認します。
例えば、MVCアーキテクチャを採用している場合、モデル、ビュー、コントローラーが適切に分離されているかをチェックします。
機能要件
- コードが仕様書に記載された機能を正しく実装しているか確認
- テストケースを使って期待される動作を検証すると効果的
非機能要件
- パフォーマンス、セキュリティ、拡張性などの非機能要件も満たしているかをチェックし
YAGNI（You Aren't Gonna Need It）の原則
- 必要な機能だけを実装し、将来の要求に備えて無駄な機能を追加しない。これはコードの複雑さを減らし、保守性を高めます。
オブジェクト指向設計の原則
- 単一責任の原則 (Single Responsibility Principle, SRP)
- クラスやモジュールが一つの責任だけを持つように設計
- オープン/クローズドの原則 (Open/Closed Principle, OCP)
- 既存のコードを修正することなく、機能を拡張
- リスコフの置換原則 (Liskov Substitution Principle, LSP)
- サブクラスが親クラスと置き換え可能
- インターフェース分離の原則 (Interface Segregation Principle, ISP)
- クライアントが使わないメソッドに依存しない
- 依存関係逆転の原則 (Dependency Inversion Principle, DIP)
- 高レベルのモジュールが低レベルのモジュールに依存しない
2. テストと品質
テストケースの完全性とカバレッジ
- すべての機能や要件がテストケースに含まれているか確認
- 未テストの部分がないようにする
- テストカバレッジツール（例えば、コードカバレッジツール）を使って、コードのどの部分がテストされていないかを確認
- ステートメントカバレッジ、ブランチカバレッジ、関数カバレッジを測定
影範囲の網羅
- ソフトウェアの異なる部分がどのように相互作用するかを理解する。それらの相互作用をテストすることが重要
- システム全体の動作を確認するテストを実施し、影響範囲が網羅されているかを確かめる
エラーの可能性やデグレのリスク
- テストケースがエラーハンドリングを含んでいるか確認する。例えば、異常入力や境界値テストを実施
- リグレッションテストを定期的に行い、以前に解決されたバグが再発しないことを確認
環境依存性の確認
- 開発環境、テスト環境、本番環境など、異なる環境でのテストを実施し、全ての環境で問題なく動作するかを確認
- 使用されるOS、ブラウザ、デバイスなどの互換性を確認
セキュリティ上の懸念
- 既知の脆弱性（例：SQLインジェクション、XSSなど）に対するテストを実施
- コードレビューやセキュリティツールを使用して、セキュリティ上の懸念がないか確認
3. チームとプロジェクトの協調
ドキュメントの更新
- 開発中のコードやシステム設計に関するドキュメントを随時更新する
再利用可能なモジュールの確認
- 新規開発に着手する前に、既存のコードベースから再利用可能なモジュールやライブラリがないかを確認する。これにより、開発効率が向上し、バグの発生リスクを低減
コードのモジュール化とリファクタリング
- 再利用性を高めるために、コードをモジュール化し、必要に応じてリファクタリングを行う。
4. リスクとリリース管理
リリース前の評価
- リリースタイミングで問題が起きないかを事前に評価する
リリース規模の評価
- リリース（PR）の規模が適切であるかを評価する。大規模なリリースはリスクが高いため、小さく分割したリリースを検討する
サービス利用規約の確認
- リリース内容がサービスの利用規約に違反していないかを確認する
5. UXとユーザー視点
画面の表示や文言に不備がないか
- 文言が誤解を招かず、明確であるか確認する。ユーザーが直感的に理解できるようにする
文言の明確さ
- 曖昧な表現を避け、具体的かつ簡潔な文言を使用する
ユーザー体験の質
- ユーザーの操作がスムーズであるか、必要な情報が容易にアクセスできるか確認する。ユーザーのフィードバックを反映し、体験を向上させる
6. 実装方法
不要になったソースコードはないか
- 使用されていない関数や変数、コメントアウトされたコードなどは削除する
DRYに書けているか
- 同じコードが複数箇所に書かれていないか確認し、共通化できる部分は共通化する
現在不要なものを実装していないか (YAGNIの原則)
- 現時点で必要のない機能を実装していないか確認する
導入したライブラリに問題はないか
- 導入したライブラリが信頼性が高く、メンテナンスされているか確認する
使えそうなライブラリはないか
- 自前で実装するよりもライブラリを使う方が効率的な場合は、それを検討する
標準のAPIにある機能を再発明していないか
- 言語の標準APIに同じ機能がある場合、それを利用するようにする
使用できる既存のモジュールはないか
- 既存のモジュールやクラスを再利用できるか検討する
APIのリクエストやレスポンスに問題はないか
- リクエストとレスポンスのフォーマット、エラーハンドリングが適切か確認する
リトライ処理は行えているか。問題ないか
- ネットワークや外部サービスの呼び出しに対してリトライ処理が適切に実装されているか確認する
データの型は適切か
- 使用しているデータ型が適切であるか、型安全性が保たれているか確認する
データの不整合は起こらないか。トランザクションは張れているか
- 複数の操作が失敗した場合でもデータが一貫性を保つようにトランザクションを使用しているか確認する
複数のテーブルにまたがる情報を更新する場合等、トランザクション処理をすべき所でトランザクションを設定しているか
- 必要な箇所でトランザクションを確実に使用しているか確認する
ログは適切に記録されているか
- エラーや重要な操作について適切にログが記録されているか確認する
デバッグ用のコードは残っていないか
- 開発中に使用したデバッグ用のコードや出力が残っていないか確認する
コメントは適切に記述されているか
- コードがわかりやすくなるように適切にコメントが記述されているか確認する
命名に問題ないか
- 変数名、関数名、クラス名が意味を持ち、規則に従っているか確認する
名前から副作用の有無は想定できるか
- 関数名やメソッド名から副作用の有無が予想できるようにする
名前と実装は合っているか
- 名前が実装内容を正確に反映しているか確認する
可読性に問題はないか
- コードが読みやすく、理解しやすいか確認する
アクセス修飾子は適切か
- クラスやメソッドのアクセス修飾子が適切に設定されているか確認する
スコープは適切か
- 変数や関数のスコープが適切に設定されているか確認する
クラスやメソッドの粒度、単一責任の原則は遵守されているか
- クラスやメソッドが単一の責任を持つように設計されているか確認する
ネストは深くないか
- 条件分岐やループのネストが深すぎないか確認します。深いネストは可読性を低下させるため、適切に分割する
7. エラー処理の実装
catch処理によるエラー情報の検知
- エラー情報がcatch処理によって適切に検知されない場合、重要なエラーメッセージが記録されない・通知されないことがあります。以下のポイントを確認する
Slackへのエラー通知
- エラーが発生した場合にSlackに通知することで、開発チームが迅速に対応できるようにする
8. パフォーマンスと最適化
負荷が高い可能性のある箇所の特定
- Webアプリケーションのパフォーマンスを監視し、どの部分で時間がかかっているのかを特定する
- 例: Chrome DevTools, NewRelic, Datadogなど
ボトルネックの分析
- 処理時間やメモリ使用量が高い箇所を特定し、最適化可能な部分を見つける
N + 1 クエリが発行されていないか
- N + 1 クエリ問題は、パフォーマンスの低下を招く大きな原因を削除する
不要なクエリの発行がされていないか
- キャッシュを利用して同じデータに対する繰り返しのクエリを減らす
計算量は問題ないか、あるいは減らせないか
- 計算量が高いアルゴリズムの処理を、効率のよいアルゴリズムに置き換える
- 例: 線形探索 (O(n)) よりもバイナリサーチ (O(log n)) を活用する場合など
9. タスク
デプロイからタスク実行までのエラー防止
-
各ステップに対して適切なエラーハンドリングを実装する
-
例：メールテンプレートの更新が失敗した場合のリトライロジック
-
ログを詳細に記録し、エラー発生時の原因追跡を容易にします（例：Lograge, Rails.logger）
-
エラー発生時に通知を行う（例：Sentry, Rollbar, New Relic）ことで、迅速な対応を可能にする
-
デプロイ前に自動テスト（例：RSpec、Capybara）を実行し、全てのケースが正常に動作することを確認する
バッチ処理設計
- 実行前にドライランを行い、実際のデータに影響を与えずにシミュレーションを実行する
- タスクの依存関係を考慮し、適切な順序で実行するように設計する
- 実行タイミングをスケジュールし、システム負荷が低い時間帯に実行します（例：夜間バッチ処理）
- 大規模なデータ処理は、分割して実行することで一度に処理するデータ量を減らし、メモリ使用量をえる
- 各ステップの途中でチェックポイントを設け、失敗時には途中から再実行可能なように設計する
- バッチ処理中のエラーに対してリカバリ処理を実装し、途中で失敗しても続行可能にする
まとめ
チェック項目多すぎて大変ですが、たまーに見返して頭にうっすら入れておくだけでも全然違うと思います。