tomevault-io 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. **いつ譲歩するか知る**
動作していて重要な問題でなければ、承認する。
完璧は進歩の敵。
ベストプラクティス
- 迅速にレビュー：24時間以内、理想的には同日
- PRサイズを制限：効果的なレビューには最大200-400行
- 時間ブロックでレビュー：最大60分、休憩を取る
- レビューツールを使用：GitHub、GitLab、または専用ツール
- できることを自動化：リンター、フォーマッター、セキュリティスキャン
- 信頼関係を構築：絵文字、賞賛、共感は重要
- 対応可能に：複雑な問題にはペアリングを提案
- 他者から学ぶ：他者のレビューコメントをレビュー
よくある落とし穴
- 完璧主義：軽微なスタイル好みで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複雑度を分析し、レビュアーを提案
Converted and distributed by TomeVault — claim your Tome and manage your conversions.