コードレビュー エクセレンス

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

このスキルを使う場合

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

基本原則

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

コードレビューの目的：

• バグと境界ケースをキャッチする
• コード保守性を確保する
• チーム全体で知識を共有する
• コーディング標準を強化する
• デザインとアーキテクチャを改善する
• チーム文化を構築する

目的ではないこと：

• 知識を見せびらかす
• フォーマットについて細かく指摘する（リンターを使用）
• 不必要に進捗をブロックする
• 個人の好みに合わせて書き直す

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

良いフィードバックの特徴：

• 具体的で行動可能
• 教育的であり、判断的ではない
• コードに焦点を当てており、人ではない
• バランスが取れている（良い成果も褒める）
• 優先順位が付いている（重要度 vs. あると良い）

❌ 悪い例: "これは間違っています。"
✅ 良い例: "複数のユーザーが同時にアクセスする場合、
レースコンディションが発生する可能性があります。
ここでミューテックスの使用を検討してください。"

❌ 悪い例: "なぜ X パターンを使わなかったのですか？"
✅ 良い例: "リポジトリパターンを検討してみませんか？
テストがしやすくなります。例はこちらです：[link]"

❌ 悪い例: "この変数を名前変更してください。"
✅ 良い例: "[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. **保守性**
   - 変数名は明確か？
   - 関数は 1 つのことだけをしているか？
   - 複雑なコードはコメントされているか？
   - マジックナンバーは抽出されているか？

フェーズ 4: サマリーと判定（2〜3 分）

1. 主な懸念事項をまとめる
2. 気に入った点をハイライトする
3. 明確な判定を下す：
   - ✅ 承認
   - 💬 コメント（軽微な提案）
   - 🔄 変更をリクエスト（対応必須）
4. 複雑な場合はペアリングを提案

レビュー技法

技法 1: チェックリスト方式

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

- [ ] ユーザー入力は検証・サニタイズされているか
- [ ] SQL クエリはパラメータ化を使用しているか
- [ ] 認証・認可チェックはされているか
- [ ] シークレットはハードコードされていないか
- [ ] エラーメッセージは情報を漏らしていないか

## パフォーマンスチェックリスト

- [ ] N+1 クエリがないか
- [ ] データベースクエリはインデックス化されているか
- [ ] 大規模リストはページネーション化されているか
- [ ] 重い操作はキャッシュされているか
- [ ] ホットパスにブロッキング I/O がないか

## テストチェックリスト

- [ ] 正常系はテストされているか
- [ ] 境界ケースはカバーされているか
- [ ] エラーケースはテストされているか
- [ ] テスト名は説明的か
- [ ] テストは決定的か

技法 2: 質問アプローチ

問題を述べるのではなく、質問を投げかけて思考を促す：

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

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

❌ 「これは非効率です。」
✅ 「すべてのユーザーをループしていることが見えます。
100k ユーザーでのパフォーマンス影響を検討しましたか？」

技法 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"Invalid value: {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('Failed to fetch user:', error);
        throw error;
    }
}

// ❌ Props の変更
function UserProfile({ user }: Props) {
    user.lastViewed = new Date();  // Props を変更している！
    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. **段階的にレビュー**
   - 第 1 PR: コア抽象化とインターフェース
   - 第 2 PR: 実装
   - 第 3 PR: 統合とテスト
   - レビューしやすく、反復が速い

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

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

// ❌ 貧弱なテスト：実装詳細をテスト
test('increments counter variable', () => {
    const component = render(<Counter />);
    const button = component.getByRole('button');
    fireEvent.click(button);
    expect(component.state.counter).toBe(1);  // 内部状態をテスト
});

// ✅ 良いテスト：動作をテスト
test('displays incremented count when clicked', () => {
    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 について説明が必要]
❓ [代案アプローチの検討]

## 判定

✅ 必須の変更対応後に承認