code-quality

コード品質

概要

保守しやすく、読みやすく、信頼性の高いコードを書くための原則と実践方法です。

---

クリーンコードの原則

意味のある名前

// ❌ Cryptic names
const d = new Date();
const u = getU();
const arr = data.filter(x => x.s === 'a');

// ✅ Descriptive names
const currentDate = new Date();
const currentUser = getCurrentUser();
const activeUsers = users.filter(user => user.status === 'active');

// ❌ Hungarian notation (outdated)
const strName = 'John';
const arrItems = [];
const bIsActive = true;

// ✅ Let the type system handle types
const name = 'John';
const items: Item[] = [];
const isActive = true;

関数

// ❌ Does too much
function processUserData(userId: string) {
  const user = db.findUser(userId);
  const orders = db.findOrders(userId);
  const total = orders.reduce((sum, o) => sum + o.amount, 0);
  sendEmail(user.email, `Your total: ${total}`);
  updateAnalytics(userId, total);
  return { user, orders, total };
}

// ✅ Single responsibility
function getUser(userId: string): User {
  return db.findUser(userId);
}

function getUserOrders(userId: string): Order[] {
  return db.findOrders(userId);
}

function calculateTotal(orders: Order[]): number {
  return orders.reduce((sum, o) => sum + o.amount, 0);
}

function sendOrderSummary(user: User, total: number): void {
  sendEmail(user.email, `Your total: ${total}`);
}

// ❌ Too many parameters
function createUser(name, email, age, role, department, manager, startDate) {}

// ✅ Use object parameter
interface CreateUserParams {
  name: string;
  email: string;
  age?: number;
  role: Role;
  department: string;
  managerId?: string;
  startDate: Date;
}

function createUser(params: CreateUserParams): User {}

コメント

// ❌ Redundant comment
// Increment counter by 1
counter++;

// ❌ Outdated comment (code changed, comment didn't)
// Returns the user's full name
function getUserEmail(user: User) {
  return user.email;
}

// ✅ Explains WHY, not WHAT
// Use binary search because the list is sorted and can have 100k+ items
const index = binarySearch(sortedItems, target);

// ✅ Warns about non-obvious behavior
// IMPORTANT: This function mutates the input array for performance reasons
function quickSort(arr: number[]): number[] {
  // ...
}

// ✅ TODO with context
// TODO(john): Remove after migration completes - tracking in JIRA-1234
const legacyAdapter = new LegacyAdapter();

---

SOLID 原則

単一責任の原則

// ❌ Multiple responsibilities
class UserManager {
  createUser(data: UserData) { /* DB logic */ }
  validateEmail(email: string) { /* Validation logic */ }
  sendWelcomeEmail(user: User) { /* Email logic */ }
  generateReport(users: User[]) { /* Report logic */ }
}

// ✅ Single responsibility each
class UserRepository {
  create(data: UserData): User { /* DB logic */ }
  findById(id: string): User | null { /* DB logic */ }
}

class UserValidator {
  validateEmail(email: string): boolean { /* Validation */ }
  validatePassword(password: string): ValidationResult { /* Validation */ }
}

class EmailService {
  sendWelcomeEmail(user: User): void { /* Email logic */ }
}

class UserReportGenerator {
  generate(users: User[]): Report { /* Report logic */ }
}

開放閉鎖の原則

// ❌ Must modify to add new payment methods
class PaymentProcessor {
  process(payment: Payment) {
    if (payment.type === 'credit') {
      // Credit card logic
    } else if (payment.type === 'paypal') {
      // PayPal logic
    } else if (payment.type === 'crypto') {
      // Crypto logic - had to modify existing code!
    }
  }
}

// ✅ Open for extension, closed for modification
interface PaymentMethod {
  process(amount: number): Promise<PaymentResult>;
}

class CreditCardPayment implements PaymentMethod {
  async process(amount: number): Promise<PaymentResult> { /* ... */ }
}

class PayPalPayment implements PaymentMethod {
  async process(amount: number): Promise<PaymentResult> { /* ... */ }
}

// New payment method - no modification to existing code
class CryptoPayment implements PaymentMethod {
  async process(amount: number): Promise<PaymentResult> { /* ... */ }
}

class PaymentProcessor {
  constructor(private method: PaymentMethod) {}

  async process(amount: number): Promise<PaymentResult> {
    return this.method.process(amount);
  }
}

リスコフの置換原則

// ❌ Violates LSP - Square breaks Rectangle contract
class Rectangle {
  constructor(public width: number, public height: number) {}

  setWidth(w: number) { this.width = w; }
  setHeight(h: number) { this.height = h; }
  getArea() { return this.width * this.height; }
}

class Square extends Rectangle {
  setWidth(w: number) {
    this.width = w;
    this.height = w; // Unexpected side effect!
  }
  setHeight(h: number) {
    this.width = h;
    this.height = h; // Unexpected side effect!
  }
}

// ✅ Proper abstraction
interface Shape {
  getArea(): number;
}

class Rectangle implements Shape {
  constructor(private width: number, private height: number) {}
  getArea() { return this.width * this.height; }
}

class Square implements Shape {
  constructor(private side: number) {}
  getArea() { return this.side * this.side; }
}

インターフェース分離の原則

// ❌ Fat interface
interface Worker {
  work(): void;
  eat(): void;
  sleep(): void;
  attendMeeting(): void;
  writeReport(): void;
}

// Robot can't eat or sleep!
class Robot implements Worker {
  work() { /* ... */ }
  eat() { throw new Error('Robots do not eat'); }  // Forced to implement
  sleep() { throw new Error('Robots do not sleep'); }
  // ...
}

// ✅ Segregated interfaces
interface Workable {
  work(): void;
}

interface Eatable {
  eat(): void;
}

interface Sleepable {
  sleep(): void;
}

class Human implements Workable, Eatable, Sleepable {
  work() { /* ... */ }
  eat() { /* ... */ }
  sleep() { /* ... */ }
}

class Robot implements Workable {
  work() { /* ... */ }
}

依存関係逆転の原則

// ❌ High-level depends on low-level
class OrderService {
  private db = new MySQLDatabase();  // Concrete dependency
  private mailer = new SendGridMailer();  // Concrete dependency

  createOrder(data: OrderData) {
    const order = this.db.insert('orders', data);
    this.mailer.send(data.email, 'Order confirmed');
    return order;
  }
}

// ✅ Depend on abstractions
interface Database {
  insert(table: string, data: unknown): unknown;
  find(table: string, query: unknown): unknown[];
}

interface Mailer {
  send(to: string, message: string): void;
}

class OrderService {
  constructor(
    private db: Database,
    private mailer: Mailer
  ) {}

  createOrder(data: OrderData) {
    const order = this.db.insert('orders', data);
    this.mailer.send(data.email, 'Order confirmed');
    return order;
  }
}

// Now we can inject any implementation
const service = new OrderService(
  new PostgresDatabase(),
  new SESMailer()
);

---

コードレビューのベストプラクティス

確認すべきポイント

## Code Review Checklist

### Correctness
- [ ] Logic is correct and handles edge cases
- [ ] Error handling is appropriate
- [ ] No obvious bugs or regressions

### Design
- [ ] Code is at the right abstraction level
- [ ] No unnecessary complexity
- [ ] Follows existing patterns in codebase

### Readability
- [ ] Clear naming and intent
- [ ] Comments explain "why" not "what"
- [ ] Code is self-documenting where possible

### Testing
- [ ] Adequate test coverage
- [ ] Tests are meaningful (not just coverage padding)
- [ ] Edge cases are tested

### Performance
- [ ] No obvious N+1 queries or inefficiencies
- [ ] Appropriate data structures used
- [ ] Caching considered if needed

### Security
- [ ] Input validation present
- [ ] No secrets in code
- [ ] Authentication/authorization correct

フィードバックの仕方

# Good Review Comments

## ✅ Specific and actionable
"This loop has O(n²) complexity. Consider using a Map for O(n) lookup."

## ✅ Explain the why
"Let's extract this to a separate function - it makes the logic easier
to test and the main function more readable."

## ✅ Offer alternatives
"Instead of mutating the array, consider using `filter()` which returns
a new array: `const active = items.filter(i => i.active)`"

## ✅ Distinguish severity
- "nit: " - Minor style issue, optional
- "suggestion: " - Good to have, not blocking
- "blocking: " - Must fix before merge

# Avoid

## ❌ Vague criticism
"This code is confusing"

## ❌ Personal attacks
"You always make this mistake"

## ❌ No explanation
"Use a different approach"

---

コードメトリクス

循環的複雑度

// High complexity (10+) - hard to test and maintain
function processOrder(order: Order): Result {
  if (order.status === 'pending') {           // +1
    if (order.paymentMethod === 'card') {     // +1
      if (order.amount > 1000) {              // +1
        // ...
      } else if (order.amount > 100) {        // +1
        // ...
      } else {
        // ...
      }
    } else if (order.paymentMethod === 'cash') {  // +1
      // ...
    }
  } else if (order.status === 'processing') {  // +1
    // ...
  }
  // ... more branches
}

// Lower complexity - extract conditions
function processOrder(order: Order): Result {
  const processor = getProcessor(order.paymentMethod);
  const tier = getPricingTier(order.amount);
  return processor.process(order, tier);
}

追跡すべきメトリクス

メトリクス	目標	理由
循環的複雑度	関数あたり 10 未満	テスト容易性
関数の長さ	50 行未満	可読性
ファイルの長さ	400 行未満	保守性
テストカバレッジ	80% 超	信頼性
重複	3% 未満	DRY 原則

---

リンティングとフォーマット

ESLint の設定

// .eslintrc.js
module.exports = {
  extends: [
    'eslint:recommended',
    'plugin:@typescript-eslint/recommended',
  ],
  rules: {
    // Prevent bugs
    'no-unused-vars': 'error',
    '@typescript-eslint/no-floating-promises': 'error',
    '@typescript-eslint/no-misused-promises': 'error',

    // Code quality
    'complexity': ['warn', 10],
    'max-lines-per-function': ['warn', 50],
    'max-depth': ['warn', 3],

    // Consistency
    'prefer-const': 'error',
    'no-var': 'error',
  }
};

プリコミットフック

// package.json
{
  "husky": {
    "hooks": {
      "pre-commit": "lint-staged"
    }
  },
  "lint-staged": {
    "*.{ts,tsx}": [
      "eslint --fix",
      "prettier --write"
    ],
    "*.{json,md}": [
      "prettier --write"
    ]
  }
}

---

関連スキル

• [[refactoring]] - 既存コードの改善
• [[testing-strategies]] - テストによる品質の確保
• [[design-patterns]] - 実証済みのソリューション

---

Sharp Edges（よくある落とし穴）

これらはコード品質において最もよくある、代償の大きい誤りです

SE-1: 過剰な設計 (Over-engineering)

• 深刻度: high
• 状況: 「将来必要になるかもしれない」ために不要な抽象化と複雑さを追加する
• 原因: YAGNI 原則の無視、デザインパターンの濫用、「万が一後で必要になったら…」という考え方
• 症状:
  • 単純な機能の変更に 10 個以上のファイルの修正が必要
  • 新入りがアーキテクチャを理解できない
  • コードが要件より 10 倍複雑
• 検出: Factory.*Factory|Abstract.*Abstract|interface.*\{.*\}(?=.*interface.*\{.*\})|Strategy.*Strategy
• 解決策: YAGNI（You Aren't Gonna Need It）、まず最も単純な実装を書く、必要になったときにリファクタリングする

SE-2: 命名の不一致

• 深刻度: medium
• 状況: 同じ概念が場所によって異なる名前で呼ばれたり、異なる概念に似た名前が使われたりする
• 原因: 用語が統一されていない、複数人開発で認識が揃っていない、コピー＆ペーストで名前を変え忘れている
• 症状:
  • user、customer、client、account が同じものを指している
  • 関連するコードを検索できない
  • 新入りがよく「これとあれは何が違うのですか？」と質問する
• 検出: (user|customer|client|account).*=.*find|(get|fetch|retrieve|load).*User
• 解決策: 用語集（Ubiquitous Language）を作成する、コードレビュー時に命名を確認する、リファクタリングで命名を統一する

SE-3: 深いネスト (Deep Nesting)

• 深刻度: medium
• 状況: if-else やコールバックのネストが 3～4 層を超え、読みにくい
• 原因: 早期 return がない、関数を抽出していない、コールバック地獄
• 症状:
  • コードを見るのに横スクロールが必要
  • どの } がどの { に対応するか追いにくい
  • 循環的複雑度が非常に高い
• 検出: \{.*\{.*\{.*\{|if.*if.*if.*if|\.then\(.*\.then\(.*\.then\(
• 解決策: ガード節（早期 return）、関数の抽出、コールバックの代わりに async/await を使う

SE-4: マジックナンバー／マジック文字列 (Magic Numbers)

• 深刻度: medium
• 状況: コード中で意味不明な数値や文字列を直接使用している
• 原因: 定数を定義するのが面倒、「一度しか使わないから抽出しなくていい」
• 症状:
  • 86400 を見ても何か分からない（1 日の秒数）
  • 変更時に全体検索して置換する必要がある
  • 同じ数値が場所によって意味は違うが値は同じ
• 検出: \b(86400|3600|1000|60000|1024|65535)\b|status\s*===?\s*['"][^'"]+['"]
• 解決策: 意味のある名前の定数として抽出する、enum を使う、設定値を一元的に管理する

SE-5: 大きな泥団子 (Big Ball of Mud)

• 深刻度: critical
• 状況: コードに明確なアーキテクチャがなく、すべてが混ざり合い、至る所に依存関係がある
• 原因: モジュール化設計がない、納期に追われて「とりあえず動けばいい」、リファクタリングの不足
• 症状:
  • A を変更すると B が壊れる
  • 単一ファイルが 1000 行以上
  • すべてがすべてを import している
• 検出: import.*from.*\.\.\/\.\.\/\.\.\/|require\(.*\.\..*\.\..*\.\.\)|lines.*>\s*1000
• 解決策: 階層アーキテクチャ、明確なモジュール境界、定期的なリファクタリング、厳格な import ルール

---

バリデーション

V-1: console.log の禁止（本番コード）

• タイプ: regex
• 深刻度: medium
• パターン: console\.(log|debug|info)\(
• メッセージ: console.log should not be in production code
• 修正提案: Use a proper logger (winston, pino) or remove debug statements
• 適用: *.ts, *.js

V-2: any 型の禁止

• タイプ: ast
• 深刻度: high
• パターン: TSAnyKeyword
• メッセージ: 'any' type defeats TypeScript's type safety
• 修正提案: Use specific type, 'unknown', or generic type parameter
• 適用: *.ts, *.tsx

V-3: 関数の引数が多すぎる

• タイプ: regex
• 深刻度: medium
• パターン: function\s+\w+\s*\([^)]*,\s*[^)]*,\s*[^)]*,\s*[^)]*,\s*[^)]*\)|=>\s*\([^)]*,\s*[^)]*,\s*[^)]*,\s*[^)]*,\s*[^)]*\)
• メッセージ: Function has more than 4 parameters - consider using an object
• 修正提案: Replace multiple params with single options object: function(options: Options)
• 適用: *.ts, *.js

V-4: TODO 無追蹤

• 類型: regex
• 嚴重度: low
• パターン: //\s*TODO(?!.*#\d|.*JIRA|.*\w+-\d+)
• メッセージ: 追跡参照のない TODO コメント
• 修復提案: 課題参照を追加してください: // TODO(#123): description またはチケットを作成してください
• 適用: *.ts, *.js, *.tsx, *.jsx

V-5: 深い import パス

• 類型: regex
• 嚴重度: medium
• パターン: import.*from\s+['"]\.\.\/\.\.\/\.\.\/|require\s*\(\s*['"]\.\.\/\.\.\/\.\.\/
• メッセージ: 深い相対 import はモジュール境界の不備を示しています
• 修復提案: パスエイリアスを使用してください: import { X } from '@/modules/x'
• 適用: *.ts, *.js, *.tsx, *.jsx