code-quality

name	code-quality
description	Code quality and refactoring best practices for maintaining clean, maintainable codebases. Use when reviewing code, refactoring, improving code structure, applying SOLID principles, reducing complexity, or improving testability. Triggers on refactor, code review, clean code, SOLID, DRY, code quality, maintainability.

Code Quality & Refactoring

Best practices for writing clean, maintainable, and high-quality code based on proven patterns from Vercel, Anthropic, and industry standards.

When to Use This Skill

• Reviewing code for quality issues
• Refactoring existing code
• Reducing complexity
• Improving testability
• Applying design patterns
• Code review feedback

Code Quality Principles

SOLID Principles

// ❌ BAD: Class doing too much (violates SRP)
class TaskManager {
  createTask(title: string) { /* ... */ }
  updateTask(id: string, data: Partial<Task>) { /* ... */ }
  deleteTask(id: string) { /* ... */ }
  sendTaskNotification(taskId: string) { /* ... */ }
  generateTaskReport(taskId: string) { /* ... */ }
  syncTaskToExternalService(taskId: string) { /* ... */ }
}

// ✅ GOOD: Single Responsibility Principle
class TaskRepository {
  create(title: string): Task { /* ... */ }
  update(id: string, data: Partial<Task>): Task { /* ... */ }
  delete(id: string): void { /* ... */ }
}

class TaskNotificationService {
  notify(taskId: string): void { /* ... */ }
}

class TaskReportGenerator {
  generate(taskId: string): Report { /* ... */ }
}

Open/Closed Principle

// ❌ BAD: Must modify class to add new behavior
class AttentionHandler {
  handle(type: AttentionType, step: AgentStep) {
    if (type === 'supervised') {
      // supervised logic
    } else if (type === 'delegated') {
      // delegated logic
    } else if (type === 'sensitive') {
      // sensitive logic
    }
    // Adding new type requires modifying this class
  }
}

// ✅ GOOD: Open for extension, closed for modification
interface AttentionStrategy {
  handle(step: AgentStep): Promise<boolean>;
}

class SupervisedStrategy implements AttentionStrategy {
  async handle(step: AgentStep): Promise<boolean> {
    // Always require approval
    return await requestUserApproval(step);
  }
}

class DelegatedStrategy implements AttentionStrategy {
  async handle(step: AgentStep): Promise<boolean> {
    // Auto-approve
    return true;
  }
}

class SensitiveStrategy implements AttentionStrategy {
  private sensitiveTools = ['run_command', 'delete_file'];
  
  async handle(step: AgentStep): Promise<boolean> {
    if (this.sensitiveTools.includes(step.toolCall?.name ?? '')) {
      return await requestUserApproval(step);
    }
    return true;
  }
}

// New attention types just add new strategy classes

Dependency Inversion

// ❌ BAD: High-level module depends on low-level details
class AgentSession {
  private sqliteDb = new SQLiteDatabase();
  
  async saveStep(step: AgentStep) {
    await this.sqliteDb.insert('agent_steps', step);
  }
}

// ✅ GOOD: Depend on abstractions
interface StepRepository {
  save(step: AgentStep): Promise<void>;
  findBySession(sessionId: string): Promise<AgentStep[]>;
}

class AgentSession {
  constructor(private stepRepository: StepRepository) {}
  
  async saveStep(step: AgentStep) {
    await this.stepRepository.save(step);
  }
}

// Now easily testable with mock repository

Function Design

Small, Focused Functions

// ❌ BAD: Function doing too much
async function processAgentResponse(response: AgentResponse) {
  // Parse response
  const parsed = JSON.parse(response.body);
  
  // Validate
  if (!parsed.type) throw new Error('Missing type');
  if (!parsed.content) throw new Error('Missing content');
  
  // Handle tool calls
  if (parsed.type === 'tool_call') {
    const result = await executeTool(parsed.toolCall);
    await saveToolResult(result);
    await notifyUser(result);
  }
  
  // Handle thinking
  if (parsed.type === 'thinking') {
    await saveThinkingStep(parsed.content);
    await streamToUI(parsed.content);
  }
  
  // Update session
  await updateSessionStatus(response.sessionId);
  await logEvent('response_processed', response);
}

// ✅ GOOD: Small, composable functions
function parseResponse(body: string): ParsedResponse {
  const parsed = JSON.parse(body);
  validateResponse(parsed);
  return parsed;
}

function validateResponse(response: unknown): asserts response is ParsedResponse {
  if (!response || typeof response !== 'object') {
    throw new ResponseError('Invalid response format');
  }
  if (!('type' in response)) {
    throw new ResponseError('Missing type field');
  }
}

async function handleToolCall(toolCall: ToolCall): Promise<ToolResult> {
  const result = await executeTool(toolCall);
  await saveToolResult(result);
  return result;
}

async function handleThinking(content: string, sessionId: string): Promise<void> {
  await saveThinkingStep(sessionId, content);
  await streamToUI(content);
}

// Composed together
async function processAgentResponse(response: AgentResponse) {
  const parsed = parseResponse(response.body);
  
  switch (parsed.type) {
    case 'tool_call':
      await handleToolCall(parsed.toolCall);
      break;
    case 'thinking':
      await handleThinking(parsed.content, response.sessionId);
      break;
  }
  
  await updateSessionStatus(response.sessionId);
}

Early Returns

// ❌ BAD: Deep nesting
function canExecuteAction(session: AgentSession, action: Action): boolean {
  if (session) {
    if (session.status === 'running') {
      if (action) {
        if (action.type === 'tool_call') {
          if (session.attentionType === 'delegated') {
            return true;
          } else if (session.attentionType === 'supervised') {
            return false; // needs approval
          }
        }
      }
    }
  }
  return false;
}

// ✅ GOOD: Early returns, flat structure
function canExecuteAction(session: AgentSession | null, action: Action | null): boolean {
  if (!session) return false;
  if (session.status !== 'running') return false;
  if (!action) return false;
  if (action.type !== 'tool_call') return false;
  
  return session.attentionType === 'delegated';
}

Naming Conventions

Intention-Revealing Names

// ❌ BAD: Cryptic names
const d = new Date();
const t = tasks.filter(t => t.s === 'p');
const x = calculateX(a, b);

// ✅ GOOD: Clear intent
const sessionStartTime = new Date();
const pendingTasks = tasks.filter(task => task.status === 'pending');
const orderIndex = calculateFractionalIndex(prevIndex, nextIndex);

Consistent Vocabulary

// ❌ BAD: Inconsistent naming
class TaskController {
  fetchTask(id: string) { /* ... */ }
  getOutcome(id: string) { /* ... */ }
  retrieveSession(id: string) { /* ... */ }
  loadStep(id: string) { /* ... */ }
}

// ✅ GOOD: Consistent vocabulary
class TaskController {
  getTask(id: string) { /* ... */ }
  getOutcome(id: string) { /* ... */ }
  getSession(id: string) { /* ... */ }
  getStep(id: string) { /* ... */ }
}

Refactoring Patterns

Extract Method

// Before: Long method with comments explaining sections
async function startSession(outcomeId: string, prompt: string) {
  // Get outcome and validate
  const outcome = await db.query.outcomes.findFirst({
    where: eq(outcomes.id, outcomeId),
    with: { task: true },
  });
  if (!outcome) throw new Error('Outcome not found');
  if (!outcome.task) throw new Error('Task not found');
  
  // Check for existing running sessions
  const existingSessions = await db.query.sessions.findMany({
    where: and(
      eq(sessions.outcomeId, outcomeId),
      eq(sessions.status, 'running')
    ),
  });
  if (existingSessions.length > 0) {
    throw new Error('Session already running');
  }
  
  // Create new session
  const session = await db.insert(sessions).values({
    id: nanoid(),
    outcomeId,
    status: 'pending',
    attentionType: outcome.task.attentionType,
    startedAt: new Date(),
  });
  
  // ... more code
}

// After: Extracted into focused methods
async function startSession(outcomeId: string, prompt: string) {
  const outcome = await getOutcomeWithTask(outcomeId);
  await ensureNoRunningSession(outcomeId);
  const session = await createSession(outcome);
  // ... rest
}

async function getOutcomeWithTask(outcomeId: string): Promise<OutcomeWithTask> {
  const outcome = await db.query.outcomes.findFirst({
    where: eq(outcomes.id, outcomeId),
    with: { task: true },
  });
  
  if (!outcome) throw new OutcomeNotFoundError(outcomeId);
  if (!outcome.task) throw new TaskNotFoundError(outcome.taskId);
  
  return outcome as OutcomeWithTask;
}

async function ensureNoRunningSession(outcomeId: string): Promise<void> {
  const running = await db.query.sessions.findFirst({
    where: and(
      eq(sessions.outcomeId, outcomeId),
      eq(sessions.status, 'running')
    ),
  });
  
  if (running) {
    throw new SessionInProgressError(running.id);
  }
}

Replace Conditional with Polymorphism

See the Open/Closed Principle example above for this pattern.

Code Quality Checklist

Structure

• Functions are small (< 30 lines preferred)
• Functions do one thing
• No duplicated code
• Related code is together
• Clear module boundaries
• Dependencies flow in one direction

Naming

• Names reveal intent
• Consistent vocabulary
• No magic numbers/strings
• Descriptive variable names

Type Safety

• Types defined for all public APIs
• No any types without justification
• Nullable types explicitly marked
• Discriminated unions for variants

Error Handling

• Specific error types used
• Errors handled at appropriate level
• No swallowed exceptions
• Helpful error messages

Testing

• Code is testable (dependencies injectable)
• Tests cover edge cases
• All tests pass

Anti-Patterns to Avoid

Anti-Pattern	Problem	Solution
God Class	One class does everything	Split into focused classes
Long Parameter List	Functions with 5+ params	Use object parameter
Primitive Obsession	Using primitives for domain concepts	Create domain types
Feature Envy	Method uses another class's data more	Move method to that class
Dead Code	Unused code cluttering	Delete it
Magic Numbers	Unexplained numeric literals	Use named constants