Code Reviewer

Expert guidance for thorough, constructive code reviews that improve code quality and team practices.

When to Use This Skill

• Reviewing pull requests
• Establishing code review standards
• Identifying code quality issues
• Security vulnerability assessment
• Performance review
• Suggesting refactoring improvements
• Enforcing coding standards
• Mentoring through code review

Code Review Checklist

Functionality

• Does the code do what it's supposed to do?
• Are edge cases handled?
• Are error cases handled appropriately?
• Does it integrate correctly with existing code?
• Are there any off-by-one errors or boundary issues?

Code Quality

• Is the code easy to understand?
• Are variable/function names descriptive?
• Is there unnecessary complexity?
• Is there duplicated code?
• Are functions/methods appropriately sized?

Security

• Is user input validated and sanitized?
• Are there SQL injection vulnerabilities?
• Are there XSS vulnerabilities?
• Are secrets/credentials hardcoded?
• Is authentication/authorization implemented correctly?

Performance

• Are there obvious performance issues?
• Are database queries efficient?
• Is caching used appropriately?
• Are there unnecessary loops or iterations?
• Is memory usage reasonable?

Testing

• Are there sufficient tests?
• Do tests cover edge cases?
• Are tests readable and maintainable?
• Do tests actually verify behavior?

Documentation

• Is complex logic documented?
• Are public APIs documented?
• Is the README updated if needed?
• Are breaking changes documented?

SOLID Principles Review

Single Responsibility Principle (SRP)

❌ Violation:

class UserService {
  async createUser(data: UserData) { /* creates user */ }
  async sendWelcomeEmail(user: User) { /* sends email */ }
  async generateReport(userId: string) { /* generates report */ }
  async uploadAvatar(userId: string, file: File) { /* uploads file */ }
}

✅ Better:

class UserService {
  async createUser(data: UserData) { /* creates user */ }
  async findUser(id: string) { /* finds user */ }
}

class EmailService {
  async sendWelcomeEmail(user: User) { /* sends email */ }
}

class ReportService {
  async generateUserReport(userId: string) { /* generates report */ }
}

class AvatarService {
  async uploadAvatar(userId: string, file: File) { /* uploads file */ }
}

Open/Closed Principle (OCP)

❌ Violation:

class PaymentProcessor {
  process(payment: Payment) {
    if (payment.type === 'credit_card') {
      // process credit card
    } else if (payment.type === 'paypal') {
      // process paypal
    } else if (payment.type === 'bitcoin') {
      // process bitcoin - added later, modifying class
    }
  }
}

✅ Better:

interface PaymentMethod {
  process(payment: Payment): Promise<Result>;
}

class CreditCardPayment implements PaymentMethod {
  async process(payment: Payment) { /* credit card logic */ }
}

class PayPalPayment implements PaymentMethod {
  async process(payment: Payment) { /* paypal logic */ }
}

// New payment methods don't require modifying existing code
class BitcoinPayment implements PaymentMethod {
  async process(payment: Payment) { /* bitcoin logic */ }
}

Liskov Substitution Principle (LSP)

❌ Violation:

class Rectangle {
  constructor(protected width: number, protected 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;
  }
}

// Breaks when substituted
function resize(rect: Rectangle) {
  rect.setWidth(5);
  rect.setHeight(10);
  console.log(rect.getArea()); // Expected 50, Square gives 100
}

✅ Better:

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 (ISP)

❌ Violation:

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

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

✅ Better:

interface Workable {
  work(): void;
}

interface Eatable {
  eat(): void;
}

interface Sleepable {
  sleep(): void;
}

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

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

Dependency Inversion Principle (DIP)

❌ Violation:

class MySQLDatabase {
  query(sql: string) { /* MySQL specific */ }
}

class UserRepository {
  private database = new MySQLDatabase(); // Direct dependency
  
  findById(id: string) {
    return this.database.query(`SELECT * FROM users WHERE id = '${id}'`);
  }
}

✅ Better:

interface Database {
  query<T>(sql: string, params?: any[]): Promise<T>;
}

class UserRepository {
  constructor(private database: Database) {} // Injected dependency
  
  findById(id: string) {
    return this.database.query('SELECT * FROM users WHERE id = ?', [id]);
  }
}

// Can use any database implementation
const repo = new UserRepository(new MySQLDatabase());
const testRepo = new UserRepository(new InMemoryDatabase());

Clean Code Principles

Naming Conventions

// Variables: descriptive, pronounceable
// ❌ Bad
const d = new Date();
const u = getUser();
const arr = items.filter(x => x > 0);

// ✅ Good
const currentDate = new Date();
const currentUser = getUser();
const positiveNumbers = items.filter(num => num > 0);

// Functions: verb + noun, describe action
// ❌ Bad
function data() { }
function process() { }
function handleIt() { }

// ✅ Good
function fetchUserProfile() { }
function calculateTotalPrice() { }
function handleFormSubmission() { }

// Booleans: is/has/can/should prefix
// ❌ Bad
const active = true;
const admin = false;

// ✅ Good
const isActive = true;
const isAdmin = false;
const hasPermission = true;
const canEdit = false;

Function Guidelines

// Keep functions small (ideally < 20 lines)
// Single level of abstraction

// ❌ Bad: Multiple abstraction levels
async function processOrder(order: Order) {
  // Validation (low level)
  if (!order.items.length) throw new Error('Empty order');
  if (!order.userId) throw new Error('No user');
  
  // Calculate total (medium level)
  let total = 0;
  for (const item of order.items) {
    total += item.price * item.quantity;
  }
  
  // Apply discount (medium level)
  if (order.coupon) {
    total *= (1 - order.coupon.discount);
  }
  
  // Save to database (low level)
  await db.orders.insert({ ...order, total });
  
  // Send email (low level)
  await emailService.send(order.userId, 'order_confirmation', { order });
}

// ✅ Good: Single level of abstraction
async function processOrder(order: Order) {
  validateOrder(order);
  const total = calculateOrderTotal(order);
  await saveOrder(order, total);
  await sendOrderConfirmation(order);
}

Avoid Magic Numbers/Strings

// ❌ Bad
if (user.role === 'admin') { }
if (retries > 3) { }
const timeout = 5000;

// ✅ Good
const ROLES = {
  ADMIN: 'admin',
  USER: 'user',
} as const;

const MAX_RETRIES = 3;
const DEFAULT_TIMEOUT_MS = 5000;

if (user.role === ROLES.ADMIN) { }
if (retries > MAX_RETRIES) { }
const timeout = DEFAULT_TIMEOUT_MS;

Error Handling

// ❌ Bad: Swallowing errors
try {
  await riskyOperation();
} catch (e) {
  // silently ignore
}

// ❌ Bad: Generic error handling
try {
  await riskyOperation();
} catch (e) {
  console.log('Error');
}

// ✅ Good: Specific error handling
try {
  await riskyOperation();
} catch (error) {
  if (error instanceof ValidationError) {
    logger.warn('Validation failed', { error: error.message });
    throw new BadRequestError(error.message);
  }
  if (error instanceof NotFoundError) {
    return null; // Expected case
  }
  logger.error('Unexpected error in riskyOperation', { error });
  throw error; // Re-throw unknown errors
}

Security Review Checklist

Input Validation

// ❌ Dangerous: No validation
app.post('/users', (req, res) => {
  db.query(`INSERT INTO users (name) VALUES ('${req.body.name}')`);
});

// ✅ Safe: Validated and parameterized
app.post('/users', (req, res) => {
  const schema = z.object({
    name: z.string().min(1).max(100).regex(/^[a-zA-Z\s]+$/),
  });
  
  const result = schema.safeParse(req.body);
  if (!result.success) {
    return res.status(422).json({ error: result.error });
  }
  
  db.query('INSERT INTO users (name) VALUES ($1)', [result.data.name]);
});

Authentication/Authorization

// ❌ Missing authorization check
app.delete('/users/:id', async (req, res) => {
  await userService.delete(req.params.id);
  res.status(204).send();
});

// ✅ With authorization
app.delete('/users/:id', 
  requireAuth,
  requirePermission('users:delete'),
  async (req, res) => {
    // Additional check: can only delete own account unless admin
    if (req.user.id !== req.params.id && !req.user.isAdmin) {
      return res.status(403).json({ error: 'Cannot delete other users' });
    }
    await userService.delete(req.params.id);
    res.status(204).send();
  }
);

Secrets Management

// ❌ Hardcoded secrets
const API_KEY = 'sk_live_abc123xyz';
const DB_PASSWORD = 'supersecret';

// ✅ Environment variables
const API_KEY = process.env.API_KEY;
const DB_PASSWORD = process.env.DB_PASSWORD;

if (!API_KEY || !DB_PASSWORD) {
  throw new Error('Missing required environment variables');
}

Performance Review

Database Queries

// ❌ N+1 Problem
const users = await db.users.findAll();
for (const user of users) {
  user.orders = await db.orders.findByUserId(user.id); // N queries!
}

// ✅ Eager loading
const users = await db.users.findAll({
  include: [{ model: Order }],
});

// ❌ Missing index
SELECT * FROM orders WHERE status = 'pending' ORDER BY created_at;

// ✅ Add index for frequent queries
CREATE INDEX idx_orders_status_created ON orders(status, created_at);

Unnecessary Work

// ❌ Recalculating in loop
items.forEach(item => {
  const tax = calculateTax(item.price); // Same calculation
  const discount = getDiscount(); // Same for all items
  process(item, tax, discount);
});

// ✅ Calculate once
const discount = getDiscount();
items.forEach(item => {
  const tax = calculateTax(item.price);
  process(item, tax, discount);
});

Review Comment Guidelines

Comment Types

🔴 Blocker: Must be fixed before merge
🟡 Suggestion: Should consider, but not required
🟢 Nitpick: Minor style/preference issue
💬 Question: Seeking clarification
📚 FYI: Educational information

Writing Good Comments

❌ Bad comments:
"This is wrong"
"Fix this"
"???"

✅ Good comments:
"This could throw if `user` is null. Consider adding a null check:
`if (!user) return null;`"

"Nice use of the factory pattern here! One suggestion: we could 
make this more flexible by accepting an options object."

"Question: Is there a reason we're not using the existing 
`formatDate` utility here? It handles timezone conversion."

Constructive Feedback

Instead of...	Say...
"This is wrong"	"This might cause X when Y happens"
"Don't do this"	"Consider X instead because..."
"Why did you...?"	"I'm curious about the reasoning for..."
"Obviously..."	(Just explain without condescension)

Code Review Process

Before Review

1. Read the PR description: Understand the context
2. Check linked issues: Know the requirements
3. Build and test locally: Verify it works
4. Review tests first: Understand expected behavior

During Review

1. Start with high-level feedback: Architecture, approach
2. Then dive into details: Implementation specifics
3. Check for completeness: Tests, docs, migrations
4. Look for patterns: Systemic issues vs. one-offs

After Review

1. Summarize feedback: Overall assessment
2. Prioritize issues: What must be fixed vs. nice-to-have
3. Offer to pair: If changes are complex
4. Re-review promptly: Don't block progress

Output Artifacts

When this skill is activated, I can help create:

1. Code Review: Detailed review with categorized feedback
2. Review Checklist: Custom checklist for specific codebases
3. Refactoring Suggestions: Specific improvements with examples
4. Security Audit: Security-focused review findings
5. Performance Analysis: Performance issue identification
6. Code Standards Document: Team coding guidelines
7. Review Guidelines: Team code review process