Code Reviewer Role

Purpose

Provides systematic code review perspective focusing on quality, security, maintainability, performance, and adherence to best practices.

When to Use

• ✅ User requests code review

• ✅ Analyzing pull requests or diffs

• ✅ Evaluating code quality of modules

• ✅ Security audit requirements

• ✅ Pre-merge quality checks

• ✅ Teaching code quality principles

Code Review Checklist

Category 1: Functionality

Questions:

• Does the code do what it's supposed to do?

• Are edge cases handled correctly?

• Are error conditions handled gracefully?

• Is the happy path clear and correct?

• Are there any logical errors?

Check For:

// ❌ Off-by-one errors for (let i = 0; i <= array.length; i++) // Should be i < array.length

// ❌ Incorrect null checks if (user.email) // Should check: if (user && user.email)

// ❌ Async/await misuse async function process() { doAsync(); // Missing await! }

// ✅ Correct error handling try { const result = await operation(); return result; } catch (error) { logger.error('Operation failed', { error, context }); throw new BusinessException('User-friendly message'); }

Search Examples:

codecompass search:semantic "error handling and exception management" codecompass search:semantic "null safety and defensive programming" codecompass search:semantic "async operations and promise handling"

Category 2: Security

OWASP Security Checks:

1. Injection Vulnerabilities

// ❌ SQL Injection const query = SELECT * FROM users WHERE email = '${userInput}';

// ✅ Parameterized queries const query = 'SELECT * FROM users WHERE email = ?'; const result = await db.execute(query, [userInput]);

// ❌ Command Injection exec(ls ${userInput});

// ✅ Sanitized input const allowedCommands = ['list', 'show', 'get']; if (!allowedCommands.includes(userInput)) { throw new Error('Invalid command'); }

2. Authentication & Authorization

// ❌ Missing authentication check @Get('/admin/users') async getUsers() { return this.userService.findAll(); }

// ✅ Protected endpoint @UseGuards(AuthGuard, RolesGuard) @Roles('admin') @Get('/admin/users') async getUsers() { return this.userService.findAll(); }

// ❌ Weak password requirements if (password.length >= 6) // Too short!

// ✅ Strong password policy const passwordRegex = /^(?=.[a-z])(?=.[A-Z])(?=.\d)(?=.[@$!%?&])[A-Za-z\d@$!%?&]{12,}$/; if (!passwordRegex.test(password)) { throw new Error('Password does not meet security requirements'); }

3. Sensitive Data Exposure

// ❌ Logging sensitive data logger.info('User login', { email, password }); // Never log passwords!

// ✅ Safe logging logger.info('User login', { email, userId });

// ❌ Exposing internal errors catch (error) { return { error: error.message, stack: error.stack }; // Leaks internals! }

// ✅ Generic error messages catch (error) { logger.error('Operation failed', { error, userId }); return { error: 'An unexpected error occurred' }; }

4. Access Control

// ❌ Insecure direct object reference (IDOR) @Get('/orders/:id') async getOrder(@Param('id') id: string) { return this.orderService.findById(id); // No ownership check! }

// ✅ Authorization check @Get('/orders/:id') async getOrder(@Param('id') id: string, @CurrentUser() user: User) { const order = await this.orderService.findById(id); if (order.userId !== user.id && !user.isAdmin) { throw new ForbiddenException('Access denied'); } return order; }

Search Examples:

codecompass search:semantic "SQL query construction with user input" codecompass search:semantic "authentication guards and authorization" codecompass search:semantic "password hashing and storage" codecompass search:semantic "logging sensitive information"

Category 3: Code Quality

1. Readability

Naming Conventions:

// ❌ Poor naming function f(x) { const y = x * 2; return y; }

// ✅ Clear naming function calculateDoublePrice(basePrice: number): number { const doublePrice = basePrice * 2; return doublePrice; }

// ❌ Ambiguous boolean let flag = false;

// ✅ Descriptive boolean let isEmailVerified = false; let hasActiveSubscription = true; let canEditOrder = user.isAdmin || order.status === 'draft';

Function Length:

// ❌ Too long (>50 lines) function processOrder() { // 100 lines of code... }

// ✅ Extracted and focused function processOrder() { validateOrder(); calculateTotal(); applyDiscounts(); processPayment(); sendConfirmation(); }

Complexity:

// ❌ High cyclomatic complexity (>10) function calculateDiscount(user, order, date) { if (user.vip) { if (order.total > 1000) { if (date.isWeekend()) { if (order.items.length > 5) { // ... more nested conditions } } } } }

// ✅ Extracted rules function calculateDiscount(user: User, order: Order, date: Date): number { const rules = [ new VipDiscountRule(), new BulkOrderDiscountRule(), new WeekendDiscountRule(), new LargeCartDiscountRule(), ];

return rules.reduce((total, rule) => total + rule.calculate(user, order, date), 0); }

2. DRY Principle (Don't Repeat Yourself)

// ❌ Duplication function processUserPayment(user: User, amount: number) { const fee = amount * 0.03; const total = amount + fee; return stripe.charge(user.paymentMethod, total); }

function processOrderPayment(order: Order) { const fee = order.total * 0.03; const total = order.total + fee; return stripe.charge(order.user.paymentMethod, total); }

// ✅ Extracted common logic function calculateFee(amount: number): number { return amount * 0.03; }

function processPayment(paymentMethod: PaymentMethod, amount: number): Promise<PaymentResult> { const fee = calculateFee(amount); const total = amount + fee; return stripe.charge(paymentMethod, total); }

3. SOLID Principles

Single Responsibility:

// ❌ Multiple responsibilities class UserService { async createUser(data: CreateUserDto) { // Validate input // Hash password // Save to database // Send welcome email // Log analytics event // Update cache } }

// ✅ Separated concerns class UserService { constructor( private readonly userRepository: UserRepository, private readonly passwordService: PasswordService, private readonly emailService: EmailService, private readonly analyticsService: AnalyticsService, ) {}

async createUser(data: CreateUserDto): Promise<User> { const hashedPassword = await this.passwordService.hash(data.password); const user = await this.userRepository.create({ ...data, password: hashedPassword });

// Delegate to other services
await this.emailService.sendWelcome(user);
await this.analyticsService.trackUserCreated(user);

return user;

} }

Open/Closed Principle:

// ❌ Modifying existing code for new features function calculateShipping(type: string, distance: number) { if (type === 'standard') { return distance * 0.5; } else if (type === 'express') { return distance * 1.5; } else if (type === 'overnight') { // New feature requires modification return distance * 3.0; } }

// ✅ Open for extension, closed for modification interface ShippingStrategy { calculate(distance: number): number; }

class StandardShipping implements ShippingStrategy { calculate(distance: number): number { return distance * 0.5; } }

class ExpressShipping implements ShippingStrategy { calculate(distance: number): number { return distance * 1.5; } }

// Add new shipping types without modifying existing code class OvernightShipping implements ShippingStrategy { calculate(distance: number): number { return distance * 3.0; } }

Dependency Inversion:

// ❌ High-level module depends on low-level module class OrderService { private stripeClient = new StripeClient(); // Direct dependency!

async processPayment(order: Order) { return this.stripeClient.charge(order.total); } }

// ✅ Both depend on abstraction interface PaymentGateway { charge(amount: number): Promise<PaymentResult>; }

class StripeGateway implements PaymentGateway { charge(amount: number): Promise<PaymentResult> { // Stripe implementation } }

class OrderService { constructor(private readonly paymentGateway: PaymentGateway) {} // Injected abstraction

async processPayment(order: Order) { return this.paymentGateway.charge(order.total); } }

Category 4: Performance

1. Database Performance

// ❌ N+1 Query Problem async function getUserOrders(userId: string) { const user = await userRepo.findById(userId); const orders = await orderRepo.findByUserId(userId);

for (const order of orders) { order.items = await orderItemRepo.findByOrderId(order.id); // N queries! }

return orders; }

// ✅ Eager loading async function getUserOrders(userId: string) { return orderRepo.find({ where: { userId }, relations: ['items'], // Single query with join }); }

// ❌ Missing index // Query: SELECT * FROM orders WHERE status = 'pending' AND created_at < NOW() - INTERVAL 1 HOUR // Table has no index on (status, created_at)

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

2. Algorithm Efficiency

// ❌ O(n²) complexity function findDuplicates(array: number[]): number[] { const duplicates = []; for (let i = 0; i < array.length; i++) { for (let j = i + 1; j < array.length; j++) { if (array[i] === array[j]) { duplicates.push(array[i]); } } } return duplicates; }

// ✅ O(n) complexity function findDuplicates(array: number[]): number[] { const seen = new Set<number>(); const duplicates = new Set<number>();

for (const num of array) { if (seen.has(num)) { duplicates.add(num); } else { seen.add(num); } }

return Array.from(duplicates); }

3. Memory Management

// ❌ Memory leak (event listeners) class DataService { constructor(private eventEmitter: EventEmitter) { this.eventEmitter.on('data', this.handleData.bind(this)); }

// No cleanup! Listener persists after service destroyed }

// ✅ Proper cleanup class DataService implements OnDestroy { private subscription: Subscription;

constructor(private eventEmitter: EventEmitter) { this.subscription = this.eventEmitter.on('data', this.handleData.bind(this)); }

onDestroy() { this.subscription.unsubscribe(); } }

// ❌ Loading entire dataset into memory async function processAllUsers() { const users = await userRepo.find(); // Could be millions! return users.map(processUser); }

// ✅ Stream processing async function processAllUsers() { const stream = await userRepo.createQueryBuilder('user').stream();

for await (const user of stream) { await processUser(user); } }

Search Examples:

codecompass search:semantic "database queries in loops" codecompass search:semantic "algorithm complexity and performance" codecompass search:semantic "memory leaks and resource cleanup"

Category 5: Testing

1. Test Coverage

// ❌ Insufficient tests describe('UserService', () => { it('should create user', async () => { const user = await service.createUser(mockData); expect(user).toBeDefined(); }); });

// ✅ Comprehensive tests describe('UserService', () => { describe('createUser', () => { it('should create user with valid data', async () => { const user = await service.createUser(validData); expect(user.email).toBe(validData.email); expect(user.password).not.toBe(validData.password); // Should be hashed });

it('should throw error for duplicate email', async () => {
  await service.createUser(validData);
  await expect(service.createUser(validData)).rejects.toThrow('Email already exists');
});

it('should send welcome email after creation', async () => {
  await service.createUser(validData);
  expect(emailService.sendWelcome).toHaveBeenCalledWith(expect.objectContaining({
    email: validData.email,
  }));
});

it('should reject invalid email format', async () => {
  const invalidData = { ...validData, email: 'invalid-email' };
  await expect(service.createUser(invalidData)).rejects.toThrow('Invalid email');
});

}); });

2. Test Quality

// ❌ Brittle test (implementation details) it('should process order', async () => { const spy = jest.spyOn(service as any, 'calculateTotal'); // Testing private method! await service.processOrder(order); expect(spy).toHaveBeenCalled(); });

// ✅ Behavior-focused test it('should include shipping fee in total', async () => { const result = await service.processOrder(order); const expectedTotal = order.subtotal + order.shippingFee; expect(result.total).toBe(expectedTotal); });

// ❌ Test with side effects it('should save user', async () => { await service.createUser(data); // Actually hits database! const users = await userRepo.find(); expect(users).toHaveLength(1); });

// ✅ Isolated test with mocks it('should save user', async () => { const mockRepo = { save: jest.fn().mockResolvedValue(mockUser) }; const service = new UserService(mockRepo as any);

await service.createUser(data);

expect(mockRepo.save).toHaveBeenCalledWith(expect.objectContaining({ email: data.email, })); });

3. Test Organization

// ❌ Unclear test structure describe('OrderService', () => { it('test1', () => {}); it('test2', () => {}); it('test3', () => {}); });

// ✅ Clear test organization (AAA pattern: Arrange, Act, Assert) describe('OrderService', () => { describe('processOrder', () => { describe('when order is valid', () => { it('should calculate total correctly', () => { // Arrange const order = createMockOrder({ subtotal: 100, tax: 10 });

    // Act
    const result = service.processOrder(order);

    // Assert
    expect(result.total).toBe(110);
  });
});

describe('when order is invalid', () => {
  it('should throw validation error', () => {
    // Arrange
    const invalidOrder = createMockOrder({ subtotal: -100 });

    // Act & Assert
    expect(() => service.processOrder(invalidOrder)).toThrow('Invalid subtotal');
  });
});

}); });

Search Examples:

codecompass search:semantic "test coverage and unit tests" codecompass search:semantic "mock objects and test doubles" codecompass search:semantic "integration tests and e2e tests"

Category 6: Maintainability

1. Documentation

// ❌ No documentation function calculate(a, b, c) { return (a + b) * c - (a * 0.1); }

// ✅ Clear documentation /**

• Calculates the total order price with discount applied.
• Formula: (basePrice + shipping) * quantity - (basePrice * discountRate)
• @param basePrice - The base price of the item in cents
• @param shipping - The shipping cost in cents
• @param quantity - Number of items ordered
• @returns The final price in cents after discount
• @example
• calculateOrderTotal(1000, 500, 2) // Returns 2900 (2 items @ $10 + $5 shipping - 10% discount) */ function calculateOrderTotal( basePrice: number, shipping: number, quantity: number, ): number { const discountRate = 0.1; return (basePrice + shipping) * quantity - (basePrice * discountRate); }

2. Error Messages

// ❌ Unclear error messages throw new Error('Invalid'); throw new Error('Error 42'); throw new Error(e.toString());

// ✅ Descriptive error messages throw new ValidationError('Email format is invalid', { field: 'email', value: userInput.email });

throw new BusinessError('Order cannot be cancelled after shipment', { orderId: order.id, currentStatus: order.status, allowedStatuses: ['pending', 'processing'], });

throw new Error( Failed to process payment: ${e.message}. + Order ID: ${order.id}, User ID: ${user.id}, );

3. Code Comments

// ❌ Obvious comments // Increment counter counter++;

// Loop through users for (const user of users) {

// ❌ Commented-out code // const oldImplementation = () => { // return someOldLogic(); // };

// ✅ Explain WHY, not WHAT // Use exponential backoff to avoid overwhelming the payment gateway // after temporary failures (their rate limit is 10 req/sec) await retryWithBackoff(processPayment, { maxRetries: 3 });

// HACK: Workaround for third-party API bug (ticket #1234) // Remove this after they deploy fix (ETA: 2025-12-01) if (response.status === 200 && !response.data) { response.data = await fetchDataManually(); }

Search Examples:

codecompass search:semantic "missing documentation and comments" codecompass search:semantic "error handling and error messages" codecompass search:semantic "commented out code blocks"

Review Process

Step 1: High-Level Pass

• Read the purpose (PR description, commit message)

• Understand the change scope

• Identify affected modules

• Check test coverage

Step 2: Deep Dive

• Review each file methodically

• Apply checklist categories

• Note issues (critical, major, minor)

• Suggest improvements

Step 3: Feedback Generation

• Start with positives

• Group related issues

• Provide examples for fixes

• Prioritize critical issues

Review Output Format

Summary

Overall: Approve with minor changes requested Complexity: Medium Risk Level: Low

Strengths

✅ Comprehensive test coverage (95%) ✅ Clear naming and documentation ✅ Good error handling

Critical Issues (Must Fix)

🔴 Security: SQL injection vulnerability in UserController:45

• Current: query = "SELECT * FROM users WHERE id = " + userId
• Fix: Use parameterized query or ORM

Major Issues (Should Fix)

🟡 Performance: N+1 query in OrderService:78

• Add eager loading for order items relation

🟡 Code Quality: Duplicated validation logic in 3 files

• Extract to shared ValidationService

Minor Issues (Consider)

🟢 Naming: Variable tmp in DiscountCalculator:12

• Suggest: discountedPrice or priceAfterDiscount

🟢 Documentation: Missing JSDoc for public method processRefund

• Add description, parameters, return type, example

Suggestions

💡 Consider using Strategy pattern for payment methods instead of switch statement 💡 Could extract magic numbers to named constants (e.g., MAX_RETRY_ATTEMPTS = 3)

Questions

❓ Why is the timeout set to 30 seconds instead of the standard 10? ❓ Should we add rate limiting to this endpoint?

Best Practices

✅ Do

• Review with empathy and respect

• Explain WHY, not just WHAT is wrong

• Provide specific examples for fixes

• Balance perfectionism with pragmatism

• Recognize good code and improvements

• Ask questions when unclear

• Focus on behavior over implementation details

❌ Don't

• Be overly critical or pedantic

• Nitpick formatting (use linter instead)

• Review your own code (get peer review)

• Approve without actually reading

• Focus only on negatives

• Demand your personal preferences

• Review when tired or rushed

Related Skills

• software-architect.md

• Architectural perspective

• semantic-search.md

• Finding code patterns

• 0-discover-capabilities.md

• Understanding codebase

Related Modules

From .ai/capabilities.json :

• ast-analyzer

• Code structure analysis

• static-analyzer

• Code quality metrics

• security-analyzer

• Vulnerability detection

• test-analyzer

• Test coverage analysis

Remember: Code review is about improving code AND helping developers grow. Be kind, be thorough, be constructive.