Code Review

Code Review Skill

Use this skill when reviewing code changes for quality, consistency, and best practices.

When to Use

• Reviewing pull requests
• Pair programming sessions
• Pre-merge code checks
• Learning from others' code
• Self-review before committing

Review Process

1. Understand Context

Before reviewing code:

• Read the PR description/ticket
• Understand the goal of the change
• Check if there are related PRs

2. High-Level Review

Ask these questions:

• Does this solve the stated problem?
• Is the approach reasonable?
• Are there simpler alternatives?
• Does it fit the existing architecture?

3. Detailed Review

Code Quality

// ❌ Bad: Unclear naming
const d = new Date();
const x = users.filter(u => u.a > d);

// ✅ Good: Self-documenting
const now = new Date();
const activeUsers = users.filter(user => user.lastActiveAt > now);

// ❌ Bad: Magic numbers
if (retries > 3) { ... }
setTimeout(fn, 86400000);

// ✅ Good: Named constants
const MAX_RETRIES = 3;
const * 60 * 60 * 1000;

if (retries > MAX_RETRIES) { ... }
setTimeout(fn, ONE_DAY_MS);

Function Design

// ❌ Bad: Function does too many things
function processUser(user) {
  // Validate (20 lines)
  // Transform (15 lines)
  // Save (10 lines)
  // Send email (15 lines)
  // Log (5 lines)
}

// ✅ Good: Single responsibility
function processUser(user) {
  validateUser(user);
  const transformed = transformUser(user);
  await saveUser(transformed);
  await notifyUser(transformed);
  logUserCreation(transformed);
}

Error Handling

// ❌ Bad: Silent failure
try {
  await riskyOperation();
} catch (e) {
  // nothing
}

// ✅ Good: Proper error handling
try {
  await riskyOperation();
} catch (error) {
  logger.error('Operation failed', { error, context });
  throw new AppError('Operation failed', { cause: error });
}

Type Safety

// ❌ Bad: Type assertions hiding issues
const user = data as User;
const value = (obj as any).property;

// ✅ Good: Type guards and validation
function isUser(data: unknown): data is User {
  return (
    typeof data === 'object' &&
    data !== null &&
    'id' in data &&
    'email' in data
  );
}

if (isUser(data)) {
  // data is safely typed as User
}

4. Check Tests

// Tests should exist for:
// - Happy path
// - Edge cases
// - Error conditions

describe('calculateDiscount', () => {
  it('should apply 10% for orders over $100', () => { ... });
  it('should return 0 for orders under $50', () => { ... });
  it('should throw for negative amounts', () => { ... });
  it('should handle zero amount', () => { ... });
});

5. Security Check

• Input validation on user data
• No secrets in code
• SQL queries are parameterized
• User output is escaped
• Authentication/authorization verified

Feedback Guidelines

Be Specific

// ❌ Bad feedback
"This code is confusing"

// ✅ Good feedback
"The variable `d` on line 45 is unclear. Consider renaming to 
`createdDate` to match the domain terminology."

Explain Why

// ❌ Bad feedback
"Don't use `any` here"

// ✅ Good feedback
"Using `any` here disables type checking for `processData`.
Consider using `unknown` with a type guard, or define a 
specific interface for the expected data shape."

Suggest Solutions

// ❌ Bad feedback
"This function is too long"

// ✅ Good feedback
"This function is 80 lines and handles validation, transformation,
and persistence. Consider extracting into smaller functions:

```typescript
function processOrder(order: Order) {
  validateOrder(order);
  const enriched = enrichOrder(order);
  return saveOrder(enriched);
}
```"

Use the Right Tone

Comment Categories

Use prefixes for clarity:

Examples

blocking: This endpoint is missing authentication. Add the 
`authenticate` middleware before the handler.

suggestion: Consider extracting this validation logic into a 
shared `validateEmail` function since it's used in 3 places.

question: I'm not familiar with this library. What's the benefit 
over the standard approach we've been using?

nit: Our style guide prefers `const` over `let` when the variable 
isn't reassigned.

praise: Nice use of the builder pattern here! This makes the 
configuration much more readable.

Review Checklist

Functionality

• Code does what it's supposed to do
• Edge cases are handled
• Error handling is appropriate
• No obvious bugs

Code Quality

• Code is readable and self-documenting
• No unnecessary complexity
• No code duplication
• Follows project conventions

Testing

• Tests exist and are meaningful
• Tests cover happy path and errors
• Tests are not flaky
• Coverage is adequate

Performance

• No obvious performance issues
• Expensive operations are optimized
• Database queries are efficient

Security

• Input is validated
• Output is escaped
• Auth is checked
• No secrets exposed

Review Output Template

## Code Review: PR #123

### Summary
[Brief overview of what was reviewed]

### Blocking Issues 🔴
1. [Issue requiring fix before merge]

### Suggestions 💡
1. [Improvement idea]
2. [Another idea]

### Questions ❓
1. [Clarification needed]

### Positive Notes ✅
- [What was done well]

### Verdict
- [ ] Approved
- [x] Request changes
- [ ] Comment only

Converted and distributed by TomeVault — claim your Tome and manage your conversions.