EXERCISE 90分

演習: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分