kent-beck-style

Kent Beck 流コードレビューのスキル

概要

このスキルでは、Kent Beck のリファクタリング哲学とシンプルな設計の原則に基づいて、コードを分析し、レビューします。コードスメルの検出、リファクタリング手法、段階的な設計改善に重点を置いています。

「コンピューターが理解できるコードなら、愚か者でも書ける。優れたプログラマーは、人間が理解できるコードを書く。」- Martin Fowler（『Refactoring』、Kent Beck による序文）

使用する場面

• ユーザーから「コードスメル」の検出や「リファクタリング」のガイダンスを求められたとき
• 「コード品質」、「可読性」、「保守性」について尋ねられたとき
• 改善の機会を見つけるためにコードをレビューするとき
• 「YAGNI」、「KISS」、「シンプルな設計」について尋ねられたとき
• 命名の改善、メソッドの抽出、重複の削除を行うとき

設計の基本原則

シンプルな設計（シンプルな設計の4つのルール）

優先順位順に並べた Kent Beck のルール：

1. テストに合格する - コードが正しく動作すること
2. 意図を明らかにする - コードが目的を明確に表現していること
3. 重複がない - DRY（同じことを繰り返さない）
4. 要素を最小限にする - クラス、メソッド、コードを最小限に抑える

YAGNI（今は必要ない）

• 実際に必要になるまで機能を追加しない
• 先を見越した汎用化を避ける
• 想像上の将来ではなく、今日の要件に合わせて構築する

// Bad: Speculative generality
interface DataProcessor<T, U, V> {
  process(input: T, options: U): V;
  processAsync(input: T, options: U): Promise<V>;
  processBatch(inputs: T[], options: U): V[];
  // We only need one sync method right now!
}

// Good: Build what you need
interface DataProcessor {
  process(input: string): Result;
}

KISS（シンプルにしておく）

• 巧妙な解決策よりシンプルな解決策を優先する
• 複雑さは保守性の敵です
• 説明が難しいなら、複雑すぎる可能性があります

段階的な設計

• 小さく安全な変更を行う
• コーディングしながら継続的にリファクタリングする
• 設計はリファクタリングを通じて形作られる

コードスメル一覧

1. 肥大化

扱いきれないほど大きくなったコードです。

長すぎるメソッド

• 多くのことを行うメソッド
• ひと目で理解するのが難しい
• リファクタリング：メソッドの抽出、一時変数をクエリに置き換える

// Bad: Long method doing many things
function processOrder(order: Order) {
  // Validate order (10 lines)
  // Calculate discount (15 lines)
  // Apply tax (10 lines)
  // Update inventory (20 lines)
  // Send notification (15 lines)
  // Generate invoice (25 lines)
}

// Good: Extracted into focused methods
function processOrder(order: Order) {
  validateOrder(order);
  const discount = calculateDiscount(order);
  const total = applyTax(order.subtotal - discount);
  updateInventory(order.items);
  sendOrderConfirmation(order);
  generateInvoice(order, total);
}

大きすぎるクラス

• 責務が多すぎるクラス
• インスタンス変数が多すぎる
• リファクタリング：クラスの抽出、サブクラスの抽出

プリミティブ型への執着

• 小さなオブジェクトではなく、プリミティブ型を過剰に使用する
• 電話番号や金額などに文字列を使用する
• リファクタリング：プリミティブ型をオブジェクトに置き換える、パラメーターオブジェクトの導入

// Bad: Primitive obsession
function createUser(
  name: string,
  email: string,
  phone: string,
  street: string,
  city: string,
  zipCode: string
) { ... }

// Good: Value objects
function createUser(
  name: Name,
  email: Email,
  phone: Phone,
  address: Address
) { ... }

長すぎるパラメーターリスト

• パラメーターが多すぎるメソッド
• 理解しにくく、正しく呼び出すのも難しい
• リファクタリング：パラメーターオブジェクトの導入、オブジェクト全体を保持する

データの塊

• 常に一緒に現れるデータのグループ
• リファクタリング：クラスの抽出、パラメーターオブジェクトの導入

2. オブジェクト指向の乱用

オブジェクト指向（OO）の原則を不適切に適用している状態です。

Switch 文

• 型に基づく switch/case や if/else の連鎖が繰り返される場合、多態性の導入が妥当なことがあります
• ケースが閉じた集合であり、短く網羅的な switch のほうが理解しやすい場合は、そのままにします
• リファクタリング：条件分岐を多態性に置き換える、型コードをサブクラスに置き換える

// Before: A type-based branch; keep it if it is already clear
function calculateArea(shape: Shape): number {
  switch (shape.type) {
    case 'circle':
      return Math.PI * shape.radius ** 2;
    case 'rectangle':
      return shape.width * shape.height;
    case 'triangle':
      return (shape.base * shape.height) / 2;
  }
}

// Alternative: Polymorphism when repeated branches need independent extension
interface Shape {
  calculateArea(): number;
}

class Circle implements Shape {
  calculateArea(): number {
    return Math.PI * this.radius ** 2;
  }
}

一時フィールド

• 特定の状況でのみ設定されるフィールド
• リファクタリング：クラスの抽出、Null オブジェクトの導入

継承の拒否

• サブクラスが継承したメソッドを使用しない
• リファクタリング：継承を委譲に置き換える

インターフェースの異なる代替クラス

• 異なるメソッド名で同じ処理を行うクラス
• リファクタリング：メソッド名の変更、スーパークラスの抽出

3. 変更を妨げる要因

変更を難しくするコードです。

発散する変更

• 1つのクラスが複数の理由で変更される
• 単一責任の原則に違反する
• リファクタリング：クラスの抽出

ショットガン手術

• 1つの変更に多くのクラスの修正が必要となる
• リファクタリング：メソッドの移動、フィールドの移動、クラスのインライン化

並行継承階層

• ある場所でサブクラスを作成すると、別の場所でもサブクラスの作成が必要になる
• リファクタリング：メソッドの移動、フィールドの移動

4. 不要な要素

必要のないコードです。

重複コード

• 複数箇所にある同じコード構造
• リファクタリング：メソッドの抽出、クラスの抽出、メソッドの引き上げ

// Bad: Duplicate code
function validateEmail(email: string): boolean {
  const regex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/;
  return regex.test(email);
}

function isValidEmail(input: string): boolean {
  const pattern = /^[^\s@]+@[^\s@]+\.[^\s@]+$/;
  return pattern.test(input);
}

// Good: Single source of truth
const EMAIL_PATTERN = /^[^\s@]+@[^\s@]+\.[^\s@]+$/;

function isValidEmail(email: string): boolean {
  return EMAIL_PATTERN.test(email);
}

デッドコード

• 到達不能または未使用のコード
• リファクタリング：デッドコードの削除

投機的一般化

• 「念のため」に作られた未使用の抽象化
• リファクタリング：階層の縮約、クラスのインライン化、パラメーターの削除

コメント（コードの臭いとして）

• コードが何をするか（理由ではなく）を説明するコメント
• 多くの場合、コードをより分かりやすくすべきであることを示しています
• リファクタリング：メソッドの抽出、メソッド名の変更

// Bad: Comment explaining what
// Check if user is adult
if (user.age >= 18) { ... }

// Good: Self-documenting code
const LEGAL_ADULT_AGE = 18;
if (user.isAdult()) { ... }

// Good use of comments: explaining why
// Using >= instead of > because legal age includes 18th birthday
if (user.age >= LEGAL_ADULT_AGE) { ... }

5. 結合

過度に結合したコードです。

機能の横恋慕

• メソッドが、自身のクラスよりも他のクラスのデータを多く使用しています
• リファクタリング：メソッドの移動、メソッドの抽出

// Bad: Feature envy
class OrderPrinter {
  print(order: Order) {
    console.log(`Order: ${order.id}`);
    console.log(`Customer: ${order.customer.name}`);
    console.log(`Total: ${order.items.reduce((sum, item) =>
      sum + item.price * item.quantity, 0)}`);
  }
}

// Good: Move calculation to Order
class Order {
  getTotal(): number {
    return this.items.reduce((sum, item) =>
      sum + item.price * item.quantity, 0);
  }
}

不適切な親密さ

• クラス同士が互いの内部実装に過度に依存しています
• リファクタリング：メソッドの移動、フィールドの移動、双方向を単方向に変更

メッセージチェーン

• メソッド呼び出しが長く連鎖しています（a.b().c().d()）
• リファクタリング：委譲の隠蔽、メソッドの抽出

ミドルマン

• 別のクラスに委譲するだけのクラスです
• リファクタリング：ミドルマンの削除、メソッドのインライン化

リファクタリング手法

メソッドの構成

メソッドの抽出

最も重要なリファクタリングです。コードの断片を、意図が分かる名前のメソッドに変換します。

// Before
function printOwing() {
  printBanner();

  // Print details
  console.log(`name: ${name}`);
  console.log(`amount: ${getOutstanding()}`);
}

// After
function printOwing() {
  printBanner();
  printDetails();
}

function printDetails() {
  console.log(`name: ${name}`);
  console.log(`amount: ${getOutstanding()}`);
}

メソッドのインライン化

メソッド本体が名前と同じくらい明確な場合に使います。

一時変数をクエリに置き換える

一時変数をメソッド呼び出しに置き換えます。

// Before
const basePrice = quantity * itemPrice;
if (basePrice > 1000) {
  return basePrice * 0.95;
}

// After
if (getBasePrice() > 1000) {
  return getBasePrice() * 0.95;
}

function getBasePrice() {
  return quantity * itemPrice;
}

機能の移動

メソッドの移動

最も頻繁に使用するクラスにメソッドを移動します。

フィールドの移動

最も頻繁に使用する場所にフィールドを移動します。

クラスの抽出

1つのクラスが、本来2つのクラスで行うべき処理を担っている場合に使います。

クラスのインライン化

クラスがほとんど何もしていない場合に使います。

データの整理

フィールドのカプセル化

フィールドをプライベートにし、アクセサーを用意します。

データ値をオブジェクトに置き換える

データ項目に追加の振る舞いが必要な場合に使います。

条件式の簡素化

条件式の分解

条件と分岐をメソッドに抽出します。

// Before
if (date.before(SUMMER_START) || date.after(SUMMER_END)) {
  charge = quantity * winterRate + winterServiceCharge;
} else {
  charge = quantity * summerRate;
}

// After
if (isWinter(date)) {
  charge = winterCharge(quantity);
} else {
  charge = summerCharge(quantity);
}

条件式の統合

同じ結果になる条件式をまとめます。

ネストした条件式をガード節に置き換える

早期リターンを使ってネストを減らします。

// Before
function getPayAmount() {
  let result;
  if (isDead) {
    result = deadAmount();
  } else {
    if (isSeparated) {
      result = separatedAmount();
    } else {
      if (isRetired) {
        result = retiredAmount();
      } else {
        result = normalPayAmount();
      }
    }
  }
  return result;
}

// After
function getPayAmount() {
  if (isDead) return deadAmount();
  if (isSeparated) return separatedAmount();
  if (isRetired) return retiredAmount();
  return normalPayAmount();
}

メソッド呼び出しの簡素化

メソッド名の変更

名前から意図が分かるようにします。

// Bad names
function calc(a, b) { ... }
function doIt() { ... }
function process(data) { ... }

// Good names
function calculateTotalPrice(items, taxRate) { ... }
function sendOrderConfirmation() { ... }
function validateUserCredentials(credentials) { ... }

パラメーターの追加／削除

パラメーターリストは最小限にし、意味のあるものにします。

パラメーターオブジェクトの導入

関連するパラメーターをまとめます。

オブジェクト全体を保持する

値を取り出す代わりに、オブジェクトを渡します。

意図が分かるコード

マジックナンバーとリテラル

分かりにくいリテラルやビジネスルールには名前を付けます。既存のプラットフォーム定数やドメイン定数を再利用し、意味がすでに明らかな慣例的なリテラルはそのまま記述します。

// Bad
if (response.status === 200) { ... }
if (password.length < 8) { ... }
const timeout = 30000;

// Good
const HTTP_OK = 200;
const MIN_PASSWORD_LENGTH = 8;
const DEFAULT_TIMEOUT_MS = 30000;

if (response.status === HTTP_OK) { ... }
if (password.length < MIN_PASSWORD_LENGTH) { ... }
const timeout = DEFAULT_TIMEOUT_MS;

命名の指針

• 変数：保持するものを表す名詞
• 関数：実行する処理を表す動詞
• ブール値：はい／いいえで答えられる質問
• クラス：表す対象を示す名詞

// Bad naming
const d = new Date();
const flag = true;
function calc() { ... }

// Good naming
const currentDate = new Date();
const isUserAuthenticated = true;
function calculateShippingCost() { ... }

自己文書化コード

コードはコメントなしで意図を表すべきです。

// Bad: Needs comment to explain
// Check if user can access premium content
if (user.subscription !== null &&
    user.subscription.endDate > new Date() &&
    user.subscription.tier === 'premium') { ... }

// Good: Self-documenting
class User {
  constructor(private subscription: { endDate: Date; tier: string } | null) {}

  hasPremiumAccess(): boolean {
    return this.subscription !== null &&
           this.subscription.endDate > new Date() &&
           this.subscription.tier === 'premium';
  }
}

// Call user.hasPremiumAccess() from the condition.

小さなステップで進める

リファクタリングのリズム

1. 小さな変更を加える
2. テストを実行する
3. テストが通ったらコミットする
4. 繰り返す

安全なリファクタリングの手順

• 一度に1つだけ変更する
• 常にコードが動作する状態を保つ
• テストに失敗したら元に戻し、さらに小さなステップで試す
• こまめにコミットする

出力形式

レビュー結果は次の形式で提示します。

# Kent Beck Style Code Review Report

## Summary
- Overall Code Health: [Good/Needs Improvement/Critical]
- Key findings summary

## Code Smells Detected

### [Smell Category]: [Specific Smell]
- **Location**: file:line
- **Description**: What the problem is
- **Impact**: Why it matters
- **Suggested Refactoring**: How to fix it

## Refactoring Recommendations

### High Priority
1. [Most impactful improvements]

### Medium Priority
1. [Moderate improvements]

### Low Priority (Nice to Have)
1. [Minor improvements]

## Positive Aspects
- What the code does well

言語別の例

TypeScript/JavaScript

// Code Smell: Primitive Obsession + Magic Numbers
function createDiscount(percent: number, startDate: string, endDate: string) {
  if (!Number.isFinite(percent) || percent < 0 || percent > 100) {
    throw new RangeError('Discount percent must be between 0 and 100');
  }
  // ...
}

// Refactored
const MIN_DISCOUNT_PERCENT = 0;
const MAX_DISCOUNT_PERCENT = 100;

class DiscountPercent {
  constructor(private value: number) {
    if (!Number.isFinite(value) || value < MIN_DISCOUNT_PERCENT || value > MAX_DISCOUNT_PERCENT) {
      throw new RangeError('Discount percent must be between 0 and 100');
    }
  }
}

class DateRange {
  constructor(
    private start: Date,
    private end: Date
  ) {
    if (!Number.isFinite(start.getTime()) || !Number.isFinite(end.getTime()) || start > end) {
      throw new RangeError('Expected valid dates with start <= end');
    }
  }
}

function createDiscount(percent: DiscountPercent, period: DateRange) { /* ... */ }

Python

# Code Smell: Long Method + Feature Envy
def process_order(order):
    # Validation (should be in Order)
    if order['items'] is None or len(order['items']) == 0:
        raise ValueError('Empty order')

    # Calculation (should be in Order)
    total = 0
    for item in order['items']:
        total += item['price'] * item['quantity']

    # More processing...

# Refactored
class Order:
    def __init__(self, items: list[OrderItem]):
        self._validate_items(items)
        self.items = items

    def _validate_items(self, items):
        if not items:
            raise EmptyOrderError()

    def calculate_total(self) -> Money:
        return sum(item.subtotal for item in self.items)

def process_order(order: Order):
    total = order.calculate_total()
    # Processing with clear responsibilities...

Java/Kotlin

// Code Smell: Switch Statement + Primitive Obsession
fun calculateShipping(type: String, weight: Double): Double {
    return when (type) {
        "standard" -> weight * 1.5
        "express" -> weight * 3.0
        "overnight" -> weight * 5.0
        else -> throw IllegalArgumentException()
    }
}

// Refactored
sealed class ShippingMethod {
    abstract fun calculateCost(weight: Weight): Money

    object Standard : ShippingMethod() {
        private const val RATE_PER_KG = 1.5
        override fun calculateCost(weight: Weight) =
            Money(weight.kilograms * RATE_PER_KG)
    }

    object Express : ShippingMethod() {
        private const val RATE_PER_KG = 3.0
        override fun calculateCost(weight: Weight) =
            Money(weight.kilograms * RATE_PER_KG)
    }

    object Overnight : ShippingMethod() {
        private const val RATE_PER_KG = 5.0
        override fun calculateCost(weight: Weight) =
            Money(weight.kilograms * RATE_PER_KG)
    }
}

参考資料

• リファクタリング：既存のコードの設計を改善する - Martin Fowler（Kent Beck と共著）
• 実装パターン - Kent Beck
• リファクタリングカタログ
• コードの臭い