code-quality

What this skill does

# Code Quality

## Overview

Principles and practices for writing maintainable, readable, and reliable code.

---

## Clean Code Principles

### Meaningful Names

```typescript
// ❌ 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;
```

### Functions

```typescript
// ❌ 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 {}
```

### Comments

```typescript
// ❌ 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 Principles

### Single Responsibility Principle

```typescript
// ❌ 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 */ }
}
```

### Open/Closed Principle

```typescript
// ❌ 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);
  }
}
```

### Liskov Substitution Principle

```typescript
// ❌ 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; }
}
```

### Interface Segregation Principle

```typescript
// ❌ 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() { /* ... */ }
}
```

### Dependency Inversion Principle

```typescript
// ❌ 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 Best Practices

### What to Look For

```markdown
## 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
```

### Giving Feedback

```markdown
# Good Review Comments

## ✅