code-review-excellence

name	code-review-excellence
description	建設的なフィードバックを提供し、早期にバグを発見し、チームの士気を維持しながら知識共有を促進する効果的なコードレビュー実践をマスター。プルリクエストのレビュー、レビュー基準の確立、開発者のメンタリング時に使用。

English | 日本語

コードレビューの卓越性

建設的なフィードバック、体系的な分析、協力的な改善を通じて、コードレビューを門番から知識共有へと変革します。

このスキルを使用するタイミング

• プルリクエストとコード変更のレビュー
• チームのコードレビュー基準の確立
• レビューを通じたジュニア開発者のメンタリング
• アーキテクチャレビューの実施
• レビューチェックリストとガイドラインの作成
• チームコラボレーションの改善
• コードレビューサイクルタイムの削減
• コード品質基準の維持

コア原則

1. レビューマインドセット

コードレビューの目標：

• バグとエッジケースを捕捉
• コードの保守性を確保
• チーム全体で知識を共有
• コーディング基準を適用
• 設計とアーキテクチャを改善
• チーム文化を構築

目標ではないこと：

• 知識を誇示
• フォーマットの細かい指摘（リンターを使用）
• 不必要に進捗をブロック
• 自分の好みに書き直す

2. 効果的なフィードバック

良いフィードバックとは：

• 具体的で実行可能
• 教育的、批判的でない
• 人ではなくコードに焦点
• バランスが取れている（良い仕事も褒める）
• 優先順位付け（重要 vs あれば良い）

❌ 悪い例：「これは間違っています。」
✅ 良い例：「複数ユーザーが同時にアクセスすると競合状態を引き起こす
         可能性があります。ここでミューテックスの使用を検討してください。」

❌ 悪い例：「なぜXパターンを使わなかったのですか？」
✅ 良い例：「リポジトリパターンを検討されましたか？テストが
         容易になります。例はこちら：[リンク]」

❌ 悪い例：「この変数名を変更してください。」
✅ 良い例：「[nit] 明確さのために`uc`ではなく`userCount`を
         検討してください。そのままにしたい場合はブロックしません。」

3. レビュー範囲

レビューすべきもの：

• ロジックの正確性とエッジケース
• セキュリティ脆弱性
• パフォーマンスへの影響
• テストカバレッジと品質
• エラーハンドリング
• ドキュメントとコメント
• API設計と命名
• アーキテクチャの適合性

手動でレビューすべきでないもの：

• コードフォーマット（Prettier、Blackなどを使用）
• インポート整理
• リンター違反
• 単純なタイポ

レビュープロセス

フェーズ1：コンテキスト収集（2-3分）

コードに飛び込む前に理解する：

1. PR説明とリンクされたissueを読む
2. PRサイズを確認（>400行？分割を依頼）
3. CI/CDステータスをレビュー（テスト合格？）
4. ビジネス要件を理解
5. 関連するアーキテクチャ決定を記録

フェーズ2：ハイレベルレビュー（5-10分）

1. **アーキテクチャと設計**
   - ソリューションは問題に適合？
   - よりシンプルなアプローチは？
   - 既存パターンと一貫性がある？
   - スケールする？

2. **ファイル構成**
   - 新規ファイルは適切な場所にある？
   - コードは論理的にグループ化されている？
   - 重複ファイルは？

3. **テスト戦略**
   - テストはある？
   - テストはエッジケースをカバー？
   - テストは読みやすい？

フェーズ3：行ごとのレビュー（10-20分）

各ファイルについて：

1. **ロジックと正確性**
   - エッジケースは処理されている？
   - オフバイワンエラーは？
   - Null/undefinedチェックは？
   - 競合状態は？

2. **セキュリティ**
   - 入力検証は？
   - SQLインジェクションリスクは？
   - XSS脆弱性は？
   - 機密データの露出は？

3. **パフォーマンス**
   - N+1クエリは？
   - 不要なループは？
   - メモリリークは？
   - ブロッキング操作は？

4. **保守性**
   - 明確な変数名？
   - 関数は一つのことをしている？
   - 複雑なコードにコメントは？
   - マジックナンバーは抽出されている？

フェーズ4：まとめと決定（2-3分）

1. 主要な懸念事項を要約
2. 良かった点を強調
3. 明確な決定を下す：
   - ✅ 承認
   - 💬 コメント（軽微な提案）
   - 🔄 変更を要求（必須対応）
4. 複雑な場合はペアリングを提案

レビューテクニック

テクニック1：チェックリスト方式

## セキュリティチェックリスト
- [ ] ユーザー入力が検証・サニタイズされている
- [ ] SQLクエリがパラメータ化されている
- [ ] 認証/認可がチェックされている
- [ ] シークレットがハードコードされていない
- [ ] エラーメッセージが情報を漏らさない

## パフォーマンスチェックリスト
- [ ] N+1クエリがない
- [ ] データベースクエリがインデックス化されている
- [ ] 大きなリストがページネーションされている
- [ ] 高コストな操作がキャッシュされている
- [ ] ホットパスにブロッキングI/Oがない

## テストチェックリスト
- [ ] ハッピーパスがテストされている
- [ ] エッジケースがカバーされている
- [ ] エラーケースがテストされている
- [ ] テスト名が説明的
- [ ] テストが決定論的

テクニック2：質問アプローチ

問題を述べる代わりに質問して思考を促す：

❌ 「リストが空の場合、これは失敗します。」
✅ 「`items`が空配列の場合、何が起こりますか？」

❌ 「ここにエラーハンドリングが必要です。」
✅ 「API呼び出しが失敗した場合、これはどう動作すべきですか？」

❌ 「これは非効率です。」
✅ 「すべてのユーザーをループしているようです。10万ユーザーで
    パフォーマンスへの影響を検討しましたか？」

テクニック3：命令ではなく提案

## 協力的な言葉遣いを使用

❌ 「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：重要度の区別

優先度を示すラベルを使用：

🔴 [blocking] - マージ前に必ず修正
🟡 [important] - 修正すべき、異議があれば議論
🟢 [nit] - あれば良い、ブロックしない
💡 [suggestion] - 検討すべき代替アプローチ
📚 [learning] - 教育的コメント、アクション不要
🎉 [praise] - 良い仕事、継続を！

例：
「🔴 [blocking] このSQLクエリはインジェクションに脆弱です。
パラメータ化クエリを使用してください。」

「🟢 [nit] 明確さのために`data`を`userData`にリネーム検討。」

「🎉 [praise] 素晴らしいテストカバレッジ！エッジケースを捕捉します。」

言語固有のパターン

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固有の問題を確認

// ❌ 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：アーキテクチャレビュー

重要な変更をレビューする際：

1. **設計ドキュメントを先に**
   - 大きな機能の場合、コード前に設計ドキュメントを要求
   - 実装前にチームで設計をレビュー
   - 手戻りを避けるためにアプローチに合意

2. **段階的にレビュー**
   - 最初のPR：コア抽象化とインターフェース
   - 2番目のPR：実装
   - 3番目のPR：統合とテスト
   - レビューが容易、反復が高速

3. **代替案を検討**
   - 「[パターン/ライブラリ]の使用を検討しましたか？」
   - 「よりシンプルなアプローチとのトレードオフは？」
   - 「要件が変わるにつれてこれはどう進化しますか？」

パターン2：テスト品質レビュー

// ❌ 貧弱なテスト：実装詳細のテスト
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：セキュリティレビュー

## セキュリティレビューチェックリスト

### 認証と認可
- [ ] 必要な場所で認証が要求されている？
- [ ] すべてのアクション前に認可チェックがある？
- [ ] JWT検証が適切（署名、有効期限）？
- [ ] APIキー/シークレットが適切に保護されている？

### 入力検証
- [ ] すべてのユーザー入力が検証されている？
- [ ] ファイルアップロードが制限されている（サイズ、タイプ）？
- [ ] SQLクエリがパラメータ化されている？
- [ ] XSS保護（出力のエスケープ）？

### データ保護
- [ ] パスワードがハッシュ化されている（bcrypt/argon2）？
- [ ] 機密データが保存時に暗号化されている？
- [ ] 機密データにHTTPSが強制されている？
- [ ] PIIが規制に従って処理されている？

### よくある脆弱性
- [ ] eval()や類似の動的実行がない？
- [ ] ハードコードされたシークレットがない？
- [ ] 状態変更操作にCSRF保護がある？
- [ ] 公開エンドポイントにレート制限がある？

難しいフィードバックの提供

パターン：サンドイッチ法（改良版）

従来型：褒める + 批判 + 褒める（わざとらしい）

より良い方法：コンテキスト + 具体的な問題 + 役立つ解決策

例：
「決済処理ロジックがコントローラー内にインライン化されている
ことに気づきました。これはテストと再利用を難しくします。

[具体的な問題]
calculateTotal()関数が税計算、割引ロジック、データベース
クエリを混在させており、単体テストと理解を困難にしています。

[役立つ解決策]
これをPaymentServiceクラスに抽出できますか？テスト可能で
再利用可能になります。必要であればペアリングできます。」

意見の不一致への対処

作成者があなたのフィードバックに同意しない場合：

1. **理解を求める**
   「あなたのアプローチを理解させてください。このパターンを
    選んだ理由は何ですか？」

2. **妥当な点を認める**
   「Xについては良い点ですね。考えていませんでした。」

3. **データを提供**
   「パフォーマンスが心配です。アプローチを検証するために
    ベンチマークを追加できますか？」

4. **必要に応じてエスカレーション**
   「[アーキテクト/シニア開発者]にこれについて意見を
    求めましょう。」

5. **いつ譲歩するか知る**
   動作していて重要な問題でなければ、承認する。
   完璧は進歩の敵。

ベストプラクティス

1. 迅速にレビュー：24時間以内、理想的には同日
2. PRサイズを制限：効果的なレビューには最大200-400行
3. 時間ブロックでレビュー：最大60分、休憩を取る
4. レビューツールを使用：GitHub、GitLab、または専用ツール
5. できることを自動化：リンター、フォーマッター、セキュリティスキャン
6. 信頼関係を構築：絵文字、賞賛、共感は重要
7. 対応可能に：複雑な問題にはペアリングを提案
8. 他者から学ぶ：他者のレビューコメントをレビュー

よくある落とし穴

• 完璧主義：軽微なスタイル好みでPRをブロック
• スコープクリープ：「ついでに、これもできますか...」
• 不一貫性：人によって異なる基準
• レビュー遅延：PRを何日も放置
• 音信不通：変更を要求してから姿を消す
• 形だけの承認：実際にレビューせずに承認
• 些細な議論：些細な詳細を広範に議論

テンプレート

PRレビューコメントテンプレート

## サマリー
[レビューした内容の簡単な概要]

## 良い点
- [うまくできていたこと]
- [良いパターンやアプローチ]

## 必須変更
🔴 [ブロッキング問題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複雑度を分析し、レビュアーを提案