演習:PRをレビューしよう
ストーリー
「ここまでの知識を実践する時が来た。 5つのPRを用意した。それぞれ異なる問題を含んでいる」
松本先輩が画面を見せた。
「実際のレビュアーとして、各PRに対して レビューコメントを書いてみろ。 プレフィックスの使い分けも忘れるなよ」
演習の進め方
以下の5つのPRのコードを読み、レビューコメントを作成してください。
各PRに対して最低3つのコメントを書きましょう。
- Must Fix / Should Fix / Nit のいずれかのプレフィックスを使用
- 問題点だけでなく、良い点も1つ以上見つけて Praise を付ける
- 具体的な改善案をコード例と共に提示
PR 1: ユーザー検索API(セキュリティの問題)
typescript
// src/controllers/userController.ts
import { Request, Response } from 'express';
import { db } from '../database';
export class UserController {
async searchUsers(req: Request, res: Response) {
const { keyword, role } = req.query;
const query = `
SELECT id, name, email, password_hash, role, created_at
FROM users
WHERE name LIKE '%${keyword}%'
AND role = '${role}'
ORDER BY created_at DESC
`;
const users = await db.raw(query);
res.json({ users, total: users.length });
}
}あなたのレビューコメントを書いてください。
<details> <summary>解答例(自分で実装してから確認しよう)</summary>[Must Fix] SQLインジェクションの脆弱性があります。
ユーザー入力 (keyword, role) を直接SQLクエリに埋め込んでいます。
パラメータ化クエリまたはORMを使用してください。
改善案:
const users = await db('users')
.select('id', 'name', 'email', 'role', 'created_at')
.where('name', 'like', `%${keyword}%`)
.andWhere('role', role)
.orderBy('created_at', 'desc');
---
[Must Fix] password_hash がレスポンスに含まれています。
パスワードハッシュは機密情報であり、APIレスポンスに
含めるべきではありません。SELECTから除外してください。
---
[Should Fix] 入力値のバリデーションがありません。
keyword が空文字や undefined の場合、
意図しない全件取得が発生します。
また、role に不正な値が渡される可能性もあります。
改善案:
if (!keyword || keyword.length < 2) {
return res.status(400).json({ error: 'Keyword must be at least 2 characters' });
}
const validRoles = ['admin', 'user', 'moderator'];
if (!validRoles.includes(role as string)) {
return res.status(400).json({ error: 'Invalid role' });
}
---
[Should Fix] ページネーションが実装されていません。
ユーザー数が多い場合、大量のデータを返すことになり
パフォーマンスが劣化します。
---
[Praise] APIのエンドポイント設計(検索パラメータをクエリ文字列で受ける)
は RESTful な設計になっています。
</details>
PR 2: 注文処理サービス(設計の問題)
typescript
// src/services/orderService.ts
export class OrderService {
async createOrder(
userId: string,
productId: string,
quantity: number,
shippingAddress: string,
paymentMethod: string,
couponCode: string | null,
giftMessage: string | null,
isExpressDelivery: boolean,
) {
// 在庫チェック
const product = await fetch(`http://inventory-service/products/${productId}`);
const productData = await product.json();
if (productData.stock < quantity) {
throw new Error('Out of stock');
}
// 金額計算
let total = productData.price * quantity;
if (couponCode) {
const coupon = await fetch(`http://coupon-service/coupons/${couponCode}`);
const couponData = await coupon.json();
total = total * (1 - couponData.discountRate);
}
if (isExpressDelivery) {
total += 1500;
}
total = total * 1.1; // 税込み
// 決済
const payment = await fetch('http://payment-service/charge', {
method: 'POST',
body: JSON.stringify({ userId, amount: total, method: paymentMethod }),
});
if (!payment.ok) {
throw new Error('Payment failed');
}
// 注文保存
const order = await fetch('http://order-db-service/orders', {
method: 'POST',
body: JSON.stringify({
userId, productId, quantity, total, shippingAddress,
giftMessage, isExpressDelivery, status: 'confirmed',
}),
});
// メール送信
await fetch('http://email-service/send', {
method: 'POST',
body: JSON.stringify({
to: userId,
template: 'order-confirmation',
data: { orderId: (await order.json()).id },
}),
});
return order.json();
}
}あなたのレビューコメントを書いてください。
<details> <summary>解答例(自分で実装してから確認しよう)</summary>[Should Fix] 引数が8つあり、パラメータリストが長すぎます。
パラメータオブジェクトへの集約を提案します。
改善案:
interface CreateOrderInput {
userId: string;
productId: string;
quantity: number;
shippingAddress: string;
paymentMethod: string;
couponCode?: string;
giftMessage?: string;
isExpressDelivery: boolean;
}
async createOrder(input: CreateOrderInput) { ... }
---
[Should Fix] この関数は在庫チェック、金額計算、決済、
注文保存、メール送信の5つの責務を持っています(神メソッド)。
関数の抽出を適用して責務を分離することを提案します。
---
[Must Fix] 決済後に注文保存やメール送信が失敗した場合、
決済だけ成功して注文が作成されない不整合が発生します。
トランザクション管理またはSagaパターンの導入を検討してください。
---
[Should Fix] マジックナンバー 1500(送料)、1.1(税率)
が関数内にハードコードされています。
定数として定義してください。
const EXPRESS_DELIVERY_FEE = 1500;
const TAX_RATE = 0.1;
---
[Should Fix] fetch のエラーハンドリングが不十分です。
ネットワークエラーや各サービスのタイムアウトを
考慮する必要があります。
try-catch で各外部呼び出しを囲み、
適切なエラーメッセージを返してください。
---
[Praise] 外部サービスとの連携が明確に分かれており、
マイクロサービスの責務分担の意図は良いと思います。
</details>
PR 3: データ変換ユーティリティ(可読性の問題)
typescript
// src/utils/transform.ts
export function t(d: any[], f: string, o?: any) {
const r: any[] = [];
for (let i = 0; i < d.length; i++) {
if (f === 'f') {
if (o.fn(d[i])) r.push(d[i]);
} else if (f === 'm') {
r.push(o.fn(d[i]));
} else if (f === 'g') {
const k = o.fn(d[i]);
const existing = r.find((x: any) => x.k === k);
if (existing) {
existing.v.push(d[i]);
} else {
r.push({ k, v: [d[i]] });
}
} else if (f === 's') {
// bubble sort
const arr = [...d];
for (let j = 0; j < arr.length; j++) {
for (let k = 0; k < arr.length - j - 1; k++) {
if (o.fn(arr[k], arr[k + 1]) > 0) {
const tmp = arr[k];
arr[k] = arr[k + 1];
arr[k + 1] = tmp;
}
}
}
return arr;
}
}
return r;
}あなたのレビューコメントを書いてください。
<details> <summary>解答例(自分で実装してから確認しよう)</summary>[Must Fix] 全てのパラメータと戻り値が any 型です。
TypeScript を使っている意味がなくなっています。
適切な型定義を追加してください。
---
[Should Fix] 関数名 `t`、引数名 `d`, `f`, `o`, `r` が
全て1文字で、何をする関数か全く分かりません。
命名を改善してください。
推測される意図:
- t → transform
- d → data / items
- f → operation / mode
- o → options
---
[Should Fix] 1つの関数でフィルター、マップ、
グルーピング、ソートの4つの操作を行っています。
JavaScript の組み込みメソッド (filter, map, sort) や
個別の関数に分割することを強く推奨します。
改善案:
// 標準の Array メソッドで十分です
const filtered = items.filter(item => item.isActive);
const mapped = items.map(item => item.name);
const sorted = items.sort((a, b) => a.price - b.price);
---
[Should Fix] ソートにバブルソートを使用しています。
計算量が O(n^2) で、大量データで性能問題が発生します。
JavaScript 組み込みの Array.sort() を使いましょう
(一般的に O(n log n) のTimSort が使われます)。
---
[Nit] 操作種別が 'f', 'm', 'g', 's' という
マジックストリングです。
もし個別関数に分割しない場合は、
enum で定義してください。
</details>
PR 4: ログインコンポーネント(テスト不足)
typescript
// src/auth/login.ts
export async function login(email: string, password: string) {
const user = await userRepository.findByEmail(email);
if (!user) {
throw new AuthenticationError('Invalid credentials');
}
const isValidPassword = await bcrypt.compare(password, user.passwordHash);
if (!isValidPassword) {
throw new AuthenticationError('Invalid credentials');
}
if (user.isLocked) {
throw new AccountLockedError('Account is locked');
}
await userRepository.updateLastLogin(user.id);
const token = jwt.sign(
{ userId: user.id, role: user.role },
process.env.JWT_SECRET!,
{ expiresIn: '24h' }
);
return { token, user: { id: user.id, name: user.name, role: user.role } };
}
// tests/auth/login.test.ts
describe('login', () => {
it('should return token for valid credentials', async () => {
const result = await login('test@example.com', 'password123');
expect(result.token).toBeDefined();
});
});あなたのレビューコメントを書いてください。
<details> <summary>解答例(自分で実装してから確認しよう)</summary>[Should Fix] テストケースが1つしかなく、
正常系のみのテストです。以下のケースを追加してください:
- 存在しないメールアドレスでログイン
- パスワードが間違っている場合
- アカウントがロックされている場合
- JWT トークンの内容が正しいことの検証
- lastLogin が更新されることの検証
---
[Should Fix] process.env.JWT_SECRET! の非nullアサーション
は危険です。環境変数が未設定の場合、実行時エラーになります。
起動時にバリデーションするか、デフォルト値を設定してください。
改善案:
const JWT_SECRET = process.env.JWT_SECRET;
if (!JWT_SECRET) {
throw new Error('JWT_SECRET environment variable is required');
}
---
[Should Fix] ログイン失敗時のレート制限がありません。
ブルートフォース攻撃に対して脆弱です。
連続失敗回数のカウントと一時ロックの実装を検討してください。
---
[Praise] ユーザーが見つからない場合とパスワードが
間違っている場合で同じエラーメッセージ 'Invalid credentials'
を返しているのは、セキュリティ上良い実装です。
(ユーザーの存在有無が分からないようにしている)
---
[Nit] JWTの有効期限 '24h' もマジックストリングです。
設定ファイルや定数に移すと変更が容易になります。
</details>
PR 5: パフォーマンスの問題
typescript
// src/services/reportService.ts
export class ReportService {
async generateMonthlyReport(year: number, month: number) {
const orders = await orderRepository.findAll();
// 対象月のフィルタ
const monthlyOrders = orders.filter(order => {
const d = new Date(order.createdAt);
return d.getFullYear() === year && d.getMonth() + 1 === month;
});
// 各注文の詳細を取得
const detailedOrders = [];
for (const order of monthlyOrders) {
const user = await userRepository.findById(order.userId);
const products = await productRepository.findByOrderId(order.id);
detailedOrders.push({ ...order, user, products });
}
// カテゴリ別集計
const categoryTotals: Record<string, number> = {};
for (const order of detailedOrders) {
for (const product of order.products) {
const category = product.category;
if (!categoryTotals[category]) {
categoryTotals[category] = 0;
}
categoryTotals[category] += product.price * product.quantity;
}
}
return {
year,
month,
totalOrders: monthlyOrders.length,
totalRevenue: monthlyOrders.reduce((sum, o) => sum + o.total, 0),
categoryBreakdown: categoryTotals,
orders: detailedOrders,
};
}
}あなたのレビューコメントを書いてください。
<details> <summary>解答例(自分で実装してから確認しよう)</summary>[Must Fix] findAll() で全注文を取得してからJSで
フィルタリングしています。データが増えるとメモリ不足と
パフォーマンス劣化が発生します。
DB側でWHERE条件を使って対象月のみ取得してください。
改善案:
const monthlyOrders = await orderRepository.findByMonth(year, month);
---
[Must Fix] N+1クエリ問題が発生しています。
monthlyOrders の件数分だけ user と products の
クエリが実行されます。月間1000件の注文があれば
2001回のDBクエリが実行されます。
JOINまたはbatch取得を使用してください。
改善案:
const userIds = monthlyOrders.map(o => o.userId);
const users = await userRepository.findByIds(userIds);
const orderIds = monthlyOrders.map(o => o.id);
const products = await productRepository.findByOrderIds(orderIds);
---
[Should Fix] カテゴリ別集計もDB側で行う方が効率的です。
GROUP BY を使えば、アプリケーション側のループが不要になります。
---
[Should Fix] レスポンスに全注文の詳細データを含めています。
レポートの用途にもよりますが、集計データだけで十分なら
detailedOrders をレスポンスから除外し、必要な場合は
別のエンドポイントで取得する設計を提案します。
---
[Praise] レポートの構造(totalOrders, totalRevenue,
categoryBreakdown)は分かりやすく、
利用者にとって使いやすいAPIレスポンスになっています。
</details>
まとめ
| PR | 主な問題カテゴリ | 重要な指摘 |
|---|---|---|
| PR 1 | セキュリティ | SQLインジェクション、パスワードハッシュの漏洩 |
| PR 2 | 設計 | 神メソッド、トランザクション不整合 |
| PR 3 | 可読性 | 不適切な命名、any 型の乱用 |
| PR 4 | テスト | テストケース不足、セキュリティ考慮 |
| PR 5 | パフォーマンス | 全件取得、N+1クエ リ |
チェックリスト
- セキュリティの観点でSQLインジェクションを指摘できた
- 設計の観点で責務分離の改善を提案できた
- 可読性の観点で命名と構造の改善を提案できた
- テストの観点で不足しているテストケースを指摘できた
- パフォーマンスの観点でN+1問題を指摘できた
- 各PRで最低1つのPraiseコメントを書けた
次のステップへ
演習を終えたら、チェックポイントクイズで理解度を確認しましょう。
推定読了時間: 90分