English | 日本語
コードレビューの卓越性
建設的なフィードバック、体系的な分析、協力的な改善を通じて、コードレビューを門番から知識共有へと変革します。
このスキルを使用するタイミング
- •プルリクエストとコード変更のレビュー
- •チームのコードレビュー基準の確立
- •レビューを通じたジュニア開発者のメンタリング
- •アーキテクチャレビューの実施
- •レビューチェックリストとガイドラインの作成
- •チームコラボレーションの改善
- •コードレビューサイクルタイムの削減
- •コード品質基準の維持
コア原則
1. レビューマインドセット
コードレビューの目標:
- •バグとエッジケースを捕捉
- •コードの保守性を確保
- •チーム全体で知識を共有
- •コーディング基準を適用
- •設計とアーキテクチャを改善
- •チーム文化を構築
目標ではないこと:
- •知識を誇示
- •フォーマットの細かい指摘(リンターを使用)
- •不必要に進捗をブロック
- •自分の好みに書き直す
2. 効果的なフィードバック
良いフィードバックとは:
- •具体的で実行可能
- •教育的、批判的でない
- •人ではなくコードに焦点
- •バランスが取れている(良い仕事も褒める)
- •優先順位付け(重要 vs あれば良い)
markdown
❌ 悪い例:「これは間違っています。」
✅ 良い例:「複数ユーザーが同時にアクセスすると競合状態を引き起こす
可能性があります。ここでミューテックスの使用を検討してください。」
❌ 悪い例:「なぜXパターンを使わなかったのですか?」
✅ 良い例:「リポジトリパターンを検討されましたか?テストが
容易になります。例はこちら:[リンク]」
❌ 悪い例:「この変数名を変更してください。」
✅ 良い例:「[nit] 明確さのために`uc`ではなく`userCount`を
検討してください。そのままにしたい場合はブロックしません。」
3. レビュー範囲
レビューすべきもの:
- •ロジックの正確性とエッジケース
- •セキュリティ脆弱性
- •パフォーマンスへの影響
- •テストカバレッジと品質
- •エラーハンドリング
- •ドキュメントとコメント
- •API設計と命名
- •アーキテクチャの適合性
手動でレビューすべきでないもの:
- •コードフォーマット(Prettier、Blackなどを使用)
- •インポート整理
- •リンター違反
- •単純なタイポ
レビュープロセス
フェーズ1:コンテキスト収集(2-3分)
markdown
コードに飛び込む前に理解する: 1. PR説明とリンクされたissueを読む 2. PRサイズを確認(>400行?分割を依頼) 3. CI/CDステータスをレビュー(テスト合格?) 4. ビジネス要件を理解 5. 関連するアーキテクチャ決定を記録
フェーズ2:ハイレベルレビュー(5-10分)
markdown
1. **アーキテクチャと設計** - ソリューションは問題に適合? - よりシンプルなアプローチは? - 既存パターンと一貫性がある? - スケールする? 2. **ファイル構成** - 新規ファイルは適切な場所にある? - コードは論理的にグループ化されている? - 重複ファイルは? 3. **テスト戦略** - テストはある? - テストはエッジケースをカバー? - テストは読みやすい?
フェーズ3:行ごとのレビュー(10-20分)
markdown
各ファイルについて: 1. **ロジックと正確性** - エッジケースは処理されている? - オフバイワンエラーは? - Null/undefinedチェックは? - 競合状態は? 2. **セキュリティ** - 入力検証は? - SQLインジェクションリスクは? - XSS脆弱性は? - 機密データの露出は? 3. **パフォーマンス** - N+1クエリは? - 不要なループは? - メモリリークは? - ブロッキング操作は? 4. **保守性** - 明確な変数名? - 関数は一つのことをしている? - 複雑なコードにコメントは? - マジックナンバーは抽出されている?
フェーズ4:まとめと決定(2-3分)
markdown
1. 主要な懸念事項を要約 2. 良かった点を強調 3. 明確な決定を下す: - ✅ 承認 - 💬 コメント(軽微な提案) - 🔄 変更を要求(必須対応) 4. 複雑な場合はペアリングを提案
レビューテクニック
テクニック1:チェックリスト方式
markdown
## セキュリティチェックリスト - [ ] ユーザー入力が検証・サニタイズされている - [ ] SQLクエリがパラメータ化されている - [ ] 認証/認可がチェックされている - [ ] シークレットがハードコードされていない - [ ] エラーメッセージが情報を漏らさない ## パフォーマンスチェックリスト - [ ] N+1クエリがない - [ ] データベースクエリがインデックス化されている - [ ] 大きなリストがページネーションされている - [ ] 高コストな操作がキャッシュされている - [ ] ホットパスにブロッキングI/Oがない ## テストチェックリスト - [ ] ハッピーパスがテストされている - [ ] エッジケースがカバーされている - [ ] エラーケースがテストされている - [ ] テスト名が説明的 - [ ] テストが決定論的
テクニック2:質問アプローチ
問題を述べる代わりに質問して思考を促す:
markdown
❌ 「リストが空の場合、これは失敗します。」
✅ 「`items`が空配列の場合、何が起こりますか?」
❌ 「ここにエラーハンドリングが必要です。」
✅ 「API呼び出しが失敗した場合、これはどう動作すべきですか?」
❌ 「これは非効率です。」
✅ 「すべてのユーザーをループしているようです。10万ユーザーで
パフォーマンスへの影響を検討しましたか?」
テクニック3:命令ではなく提案
markdown
## 協力的な言葉遣いを使用
❌ 「async/awaitに変更する必要があります」
✅ 「提案:async/awaitを使うと読みやすくなるかもしれません:
```typescript
async function fetchUser(id: string) {
const user = await db.query('SELECT * FROM users WHERE id = ?', id);
return user;
}
```
どう思いますか?」
❌ 「これを関数に抽出してください」
✅ 「このロジックは3箇所に現れています。共有ユーティリティ
関数に抽出するのは意味がありますか?」
テクニック4:重要度の区別
markdown
優先度を示すラベルを使用: 🔴 [blocking] - マージ前に必ず修正 🟡 [important] - 修正すべき、異議があれば議論 🟢 [nit] - あれば良い、ブロックしない 💡 [suggestion] - 検討すべき代替アプローチ 📚 [learning] - 教育的コメント、アクション不要 🎉 [praise] - 良い仕事、継続を! 例: 「🔴 [blocking] このSQLクエリはインジェクションに脆弱です。 パラメータ化クエリを使用してください。」 「🟢 [nit] 明確さのために`data`を`userData`にリネーム検討。」 「🎉 [praise] 素晴らしいテストカバレッジ!エッジケースを捕捉します。」
言語固有のパターン
Pythonコードレビュー
python
# Python固有の問題を確認
# ❌ ミュータブルなデフォルト引数
def add_item(item, items=[]): # バグ!呼び出し間で共有される
items.append(item)
return items
# ✅ Noneをデフォルトに使用
def add_item(item, items=None):
if items is None:
items = []
items.append(item)
return items
# ❌ 広すぎるキャッチ
try:
result = risky_operation()
except: # KeyboardInterruptまで捕捉してしまう!
pass
# ✅ 特定の例外を捕捉
try:
result = risky_operation()
except ValueError as e:
logger.error(f\"無効な値: {e}\")
raise
# ❌ ミュータブルなクラス属性の使用
class User:
permissions = [] # すべてのインスタンスで共有される!
# ✅ __init__で初期化
class User:
def __init__(self):
self.permissions = []
TypeScript/JavaScriptコードレビュー
typescript
// TypeScript固有の問題を確認
// ❌ anyを使うと型安全性が台無し
function processData(data: any) { // anyを避ける
return data.value;
}
// ✅ 適切な型を使用
interface DataPayload {
value: string;
}
function processData(data: DataPayload) {
return data.value;
}
// ❌ 非同期エラーを処理していない
async function fetchUser(id: string) {
const response = await fetch(`/api/users/${id}`);
return response.json(); // ネットワーク失敗時はどうなる?
}
// ✅ エラーを適切に処理
async function fetchUser(id: string): Promise<User> {
try {
const response = await fetch(`/api/users/${id}`);
if (!response.ok) {
throw new Error(`HTTP ${response.status}`);
}
return await response.json();
} catch (error) {
console.error('ユーザー取得失敗:', error);
throw error;
}
}
// ❌ propsのミューテーション
function UserProfile({ user }: Props) {
user.lastViewed = new Date(); // propをミューテート!
return <div>{user.name}</div>;
}
// ✅ propsをミューテートしない
function UserProfile({ user, onView }: Props) {
useEffect(() => {
onView(user.id); // 親に更新を通知
}, [user.id]);
return <div>{user.name}</div>;
}
高度なレビューパターン
パターン1:アーキテクチャレビュー
markdown
重要な変更をレビューする際: 1. **設計ドキュメントを先に** - 大きな機能の場合、コード前に設計ドキュメントを要求 - 実装前にチームで設計をレビュー - 手戻りを避けるためにアプローチに合意 2. **段階的にレビュー** - 最初のPR:コア抽象化とインターフェース - 2番目のPR:実装 - 3番目のPR:統合とテスト - レビューが容易、反復が高速 3. **代替案を検討** - 「[パターン/ライブラリ]の使用を検討しましたか?」 - 「よりシンプルなアプローチとのトレードオフは?」 - 「要件が変わるにつれてこれはどう進化しますか?」
パターン2:テスト品質レビュー
typescript
// ❌ 貧弱なテスト:実装詳細のテスト
test('カウンター変数を増加', () => {
const component = render(<Counter />);
const button = component.getByRole('button');
fireEvent.click(button);
expect(component.state.counter).toBe(1); // 内部状態をテスト
});
// ✅ 良いテスト:動作のテスト
test('クリック時に増加したカウントを表示', () => {
render(<Counter />);
const button = screen.getByRole('button', { name: /increment/i });
fireEvent.click(button);
expect(screen.getByText('Count: 1')).toBeInTheDocument();
});
// テストのレビュー質問:
// - テストは実装ではなく動作を説明している?
// - テスト名は明確で説明的?
// - テストはエッジケースをカバーしている?
// - テストは独立している(共有状態なし)?
// - テストは任意の順序で実行可能?
パターン3:セキュリティレビュー
markdown
## セキュリティレビューチェックリスト ### 認証と認可 - [ ] 必要な場所で認証が要求されている? - [ ] すべてのアクション前に認可チェックがある? - [ ] JWT検証が適切(署名、有効期限)? - [ ] APIキー/シークレットが適切に保護されている? ### 入力検証 - [ ] すべてのユーザー入力が検証されている? - [ ] ファイルアップロードが制限されている(サイズ、タイプ)? - [ ] SQLクエリがパラメータ化されている? - [ ] XSS保護(出力のエスケープ)? ### データ保護 - [ ] パスワードがハッシュ化されている(bcrypt/argon2)? - [ ] 機密データが保存時に暗号化されている? - [ ] 機密データにHTTPSが強制されている? - [ ] PIIが規制に従って処理されている? ### よくある脆弱性 - [ ] eval()や類似の動的実行がない? - [ ] ハードコードされたシークレットがない? - [ ] 状態変更操作にCSRF保護がある? - [ ] 公開エンドポイントにレート制限がある?
難しいフィードバックの提供
パターン:サンドイッチ法(改良版)
markdown
従来型:褒める + 批判 + 褒める(わざとらしい) より良い方法:コンテキスト + 具体的な問題 + 役立つ解決策 例: 「決済処理ロジックがコントローラー内にインライン化されている ことに気づきました。これはテストと再利用を難しくします。 [具体的な問題] calculateTotal()関数が税計算、割引ロジック、データベース クエリを混在させており、単体テストと理解を困難にしています。 [役立つ解決策] これをPaymentServiceクラスに抽出できますか?テスト可能で 再利用可能になります。必要であればペアリングできます。」
意見の不一致への対処
markdown
作成者があなたのフィードバックに同意しない場合:
1. **理解を求める**
「あなたのアプローチを理解させてください。このパターンを
選んだ理由は何ですか?」
2. **妥当な点を認める**
「Xについては良い点ですね。考えていませんでした。」
3. **データを提供**
「パフォーマンスが心配です。アプローチを検証するために
ベンチマークを追加できますか?」
4. **必要に応じてエスカレーション**
「[アーキテクト/シニア開発者]にこれについて意見を
求めましょう。」
5. **いつ譲歩するか知る**
動作していて重要な問題でなければ、承認する。
完璧は進歩の敵。
ベストプラクティス
- •迅速にレビュー:24時間以内、理想的には同日
- •PRサイズを制限:効果的なレビューには最大200-400行
- •時間ブロックでレビュー:最大60分、休憩を取る
- •レビューツールを使用:GitHub、GitLab、または専用ツール
- •できることを自動化:リンター、フォーマッター、セキュリティスキャン
- •信頼関係を構築:絵文字、賞賛、共感は重要
- •対応可能に:複雑な問題にはペアリングを提案
- •他者から学ぶ:他者のレビューコメントをレビュー
よくある落とし穴
- •完璧主義:軽微なスタイル好みでPRをブロック
- •スコープクリープ:「ついでに、これもできますか...」
- •不一貫性:人によって異なる基準
- •レビュー遅延:PRを何日も放置
- •音信不通:変更を要求してから姿を消す
- •形だけの承認:実際にレビューせずに承認
- •些細な議論:些細な詳細を広範に議論
テンプレート
PRレビューコメントテンプレート
markdown
## サマリー [レビューした内容の簡単な概要] ## 良い点 - [うまくできていたこと] - [良いパターンやアプローチ] ## 必須変更 🔴 [ブロッキング問題1] 🔴 [ブロッキング問題2] ## 提案 💡 [改善1] 💡 [改善2] ## 質問 ❓ [Xについての明確化が必要] ❓ [代替アプローチの検討] ## 結論 ✅ 必須変更対応後に承認
リソース
- •references/code-review-best-practices.md:包括的なレビューガイドライン
- •references/common-bugs-checklist.md:言語固有の注意すべきバグ
- •references/security-review-guide.md:セキュリティ重視のレビューチェックリスト
- •assets/pr-review-template.md:標準レビューコメントテンプレート
- •assets/review-checklist.md:クイックリファレンスチェックリスト
- •scripts/pr-analyzer.py:PR複雑度を分析し、レビュアーを提案