Skill code-quality

Code Quality

This skill provides code quality principles and best practices for reviewing and improving implementations.

---

SOLID Principles

Single Responsibility Principle (SRP)

A class/function should have one reason to change.

Bad:

class UserService {
  createUser(data) { /* creates user */ }
  sendEmail(user) { /* sends email */ }
  generateReport(users) { /* generates report */ }
}

Good:

class UserService {
  createUser(data) { /* creates user */ }
}
class EmailService {
  sendEmail(user) { /* sends email */ }
}
class ReportService {
  generateReport(users) { /* generates report */ }
}

Open/Closed Principle (OCP)

Open for extension, closed for modification.

Bad: Adding new payment types requires modifying existing code

function processPayment(type, amount) {
  if (type === 'credit') { /* ... */ }
  else if (type === 'debit') { /* ... */ }
  // Must modify to add new types
}

Good: New payment types can be added without modification

interface PaymentProcessor {
  process(amount: number): Promise<void>;
}
class CreditProcessor implements PaymentProcessor { /* ... */ }
class DebitProcessor implements PaymentProcessor { /* ... */ }

Liskov Substitution Principle (LSP)

Subtypes must be substitutable for their base types.

Bad:

class Bird {
  fly() { /* ... */ }
}
class Penguin extends Bird {
  fly() { throw new Error("Can't fly!"); } // Violates LSP
}

Good:

class Bird { /* ... */ }
class FlyingBird extends Bird {
  fly() { /* ... */ }
}
class Penguin extends Bird { /* no fly method */ }

Interface Segregation Principle (ISP)

Clients shouldn't depend on interfaces they don't use.

Bad:

interface Worker {
  work(): void;
  eat(): void;
  sleep(): void;
}

Good:

interface Workable { work(): void; }
interface Eatable { eat(): void; }
interface Sleepable { sleep(): void; }

Dependency Inversion Principle (DIP)

Depend on abstractions, not concretions.

Bad:

class UserService {
  private db = new PostgresDatabase();
}

Good:

class UserService {
  constructor(private db: Database) {}
}

---

DRY, KISS, YAGNI

DRY (Don't Repeat Yourself)

Every piece of knowledge should have a single representation.

Apply when:

• Same logic appears 3+ times
• Changes to one place require changes to others
• Bug fixes need to be applied in multiple places

Don't over-apply:

• Two similar things might diverge later
• Premature abstraction can be worse than duplication

KISS (Keep It Simple, Stupid)

Prefer simple solutions over clever ones.

Simple code:

• Easy to read and understand
• Easy to debug
• Easy to modify
• Has fewer bugs

YAGNI (You Aren't Gonna Need It)

Don't add functionality until it's needed.

Avoid:

• Building for hypothetical requirements
• Adding "just in case" features
• Over-engineering for scale you don't have

---

Clean Code Principles

Meaningful Names

// Bad
const d = new Date();
const arr = users.filter(u => u.a > 18);

// Good
const currentDate = new Date();
const adultUsers = users.filter(user => user.age > 18);

Small Functions

• Do one thing well
• Few parameters (ideally 0-3)
• Single level of abstraction

Avoid Side Effects

// Bad: side effect hidden in getter
getUser() {
  this.lastAccess = Date.now(); // Side effect!
  return this.user;
}

// Good: explicit about what it does
getUser() {
  return this.user;
}
recordAccess() {
  this.lastAccess = Date.now();
}

Error Handling

// Be specific about errors
class ValidationError extends Error {}
class NotFoundError extends Error {}
class AuthorizationError extends Error {}

// Handle at appropriate level
try {
  await processOrder(order);
} catch (error) {
  if (error instanceof ValidationError) {
    return res.status(400).json({ error: error.message });
  }
  if (error instanceof NotFoundError) {
    return res.status(404).json({ error: error.message });
  }
  throw error; // Re-throw unexpected errors
}

---

Testing Strategies

Test Pyramid

      /\
     /  \      E2E Tests (few)
    /────\
   /      \    Integration Tests (some)
  /────────\
 /          \  Unit Tests (many)
/────────────\

Unit Testing

• Test individual functions/classes
• Mock dependencies
• Fast and isolated

describe('calculateTotal', () => {
  it('sums item prices', () => {
    const items = [{ price: 10 }, { price: 20 }];
    expect(calculateTotal(items)).toBe(30);
  });

  it('applies discount', () => {
    const items = [{ price: 100 }];
    expect(calculateTotal(items, { discount: 0.1 })).toBe(90);
  });

  it('handles empty cart', () => {
    expect(calculateTotal([])).toBe(0);
  });
});

Integration Testing

• Test component interactions
• Use real dependencies (or test doubles)
• Database, API integration

Test Behavior, Not Implementation

// Bad: tests implementation details
it('calls _internalMethod', () => {
  const spy = jest.spyOn(service, '_internalMethod');
  service.doThing();
  expect(spy).toHaveBeenCalled();
});

// Good: tests behavior
it('sends welcome email on registration', async () => {
  await service.registerUser({ email: 'test@test.com' });
  expect(emailSent).toContainEqual({
    to: 'test@test.com',
    template: 'welcome'
  });
});

Edge Cases to Test

• Empty inputs
• Null/undefined
• Boundary values
• Error conditions
• Concurrent operations
• Large inputs

---

Code Review Checklist

Correctness

• Does the code do what it's supposed to?
• Are edge cases handled?
• Are error conditions handled?

Security

• Is input validated?
• Are outputs escaped?
• Are secrets protected?

Performance

• Are there N+1 queries?
• Are there unnecessary loops?
• Is caching used appropriately?

Maintainability

• Is the code readable?
• Are names meaningful?
• Is complexity reasonable?
• Is there appropriate documentation?

Testing

• Are there tests?
• Do tests cover edge cases?
• Are tests maintainable?

---

Common Code Smells

Smell	Description	Solution
Long Method	Function > 20-30 lines	Extract methods
Large Class	Class with too many responsibilities	Split into focused classes
Long Parameter List	> 3-4 parameters	Use parameter object
Duplicate Code	Same code in multiple places	Extract function
Dead Code	Unused code	Delete it
Magic Numbers	Unexplained numeric literals	Use named constants
Nested Conditionals	Deep if/else nesting	Early returns, extract methods
Feature Envy	Method uses another class's data heavily	Move method to that class

---

Refactoring Techniques

Extract Function

When a code block can be grouped and named

Inline Function

When the function body is as clear as the name

Extract Variable

When an expression is complex

Rename Variable/Function

When the name doesn't communicate intent

Replace Conditional with Polymorphism

When you have repeated switch/if statements on type

Introduce Parameter Object

When several parameters travel together