Code Review Best Practices

// ✅ Code Review Checklist Template

/**
 * PR Review Checklist
 * 
 * FUNCTIONALITY
 * [ ] Does the code solve the problem described in the PR?
 * [ ] Are edge cases handled?
 * [ ] Are error cases handled gracefully?
 * [ ] Does it integrate correctly with existing code?
 * [ ] Are there any breaking changes? If so, are they documented?
 * 
 * CODE QUALITY
 * [ ] Is the code readable and maintainable?
 * [ ] Are variable/function names descriptive?
 * [ ] Is there code duplication that should be extracted?
 * [ ] Are there magic numbers/strings that should be constants?
 * [ ] Is the code following project conventions?
 * 
 * PERFORMANCE
 * [ ] Are there any obvious performance issues?
 * [ ] Are database queries optimized?
 * [ ] Are unnecessary re-renders avoided?
 * [ ] Is memory usage reasonable?
 * 
 * TESTING
 * [ ] Are there adequate tests?
 * [ ] Do tests cover edge cases?
 * [ ] Are tests maintainable and readable?
 * [ ] Do tests actually test what they claim to test?
 * 
 * DOCUMENTATION
 * [ ] Is complex logic documented?
 * [ ] Are public APIs documented?
 * [ ] Is the PR description clear?
 * [ ] Are breaking changes documented?
 * 
 * SECURITY
 * [ ] Are inputs validated and sanitized?
 * [ ] Are there any security vulnerabilities?
 * [ ] Are secrets properly handled?
 * [ ] Are permissions checked?
 */

// ✅ Review Timing Best Practices

/**
 * Review Timing Guidelines
 */

// ✅ GOOD: Set clear expectations
const REVIEW_SLA = {
  small: "2-4 hours",      // < 100 lines
  medium: "4-8 hours",     // 100-500 lines
  large: "1 business day", // > 500 lines
  urgent: "1-2 hours"      // Hotfixes, critical bugs
};

// ✅ GOOD: Acknowledge quickly
async function acknowledgePR(pr: PullRequest) {
  // Even if you can't review immediately, acknowledge
  await comment(pr, "Thanks for the PR! I'll review this by EOD.");
  
  // Or request more time if needed
  if (pr.isLarge()) {
    await comment(pr, "This is a large PR. I'll need until tomorrow to review thoroughly.");
  }
}

// ✅ GOOD: Block time for reviews
const REVIEW_SCHEDULE = {
  morning: "9-10 AM: Review queue",
  afternoon: "2-3 PM: Review queue",
  endOfDay: "Review any remaining PRs"
};

// ❌ BAD: Let PRs sit for days
// ❌ BAD: Review without understanding context
// ❌ BAD: Rush through reviews to meet SLA

// ✅ GOOD: Use review tools effectively
const REVIEW_TOOLS = {
  github: "Use 'Request changes' for blocking issues",
  gitlab: "Use 'Needs work' for must-fix items",
  bitbucket: "Use 'Needs work' appropriately"
};

// ✅ GOOD: Batch similar reviews
// If multiple PRs touch the same area, review them together
// to understand the full context

// ✅ Feedback Template: Code Quality Issue

/**
 * Template: Addressing Code Quality
 */

// Example 1: Code Duplication
const feedback1 = `Great work on this feature! I noticed that the validation 
logic in UserService.validateEmail() is very similar to the logic in 
UserService.validatePhone(). 

Consider extracting this into a shared validator function to reduce 
duplication and make future changes easier:

```typescript
function validateFormat(value: string, pattern: RegExp): boolean {
  return pattern.test(value);
}

// Then use it in both places
validateFormat(email, EMAIL_REGEX);
validateFormat(phone, PHONE_REGEX);
```

This would make the code more maintainable. What do you think?`;

// Example 2: Performance Concern
const feedback2 = `Nice implementation! I'm concerned about the performance 
of this nested loop (lines 45-52). With 1000 users and 1000 orders, 
this would be O(n*m) = 1,000,000 operations.

Consider using a Map for O(1) lookups:

```typescript
const userMap = new Map(users.map(u => [u.id, u]));
orders.forEach(order => {
  const user = userMap.get(order.userId);
  // ...
});
```

This would reduce it to O(n+m). Would this work for your use case?`;

// Example 3: Naming Suggestion
const feedback3 = `The function works well! The name `processData` is a 
bit generic. Since it's specifically calculating user statistics, 
consider renaming it to `calculateUserStatistics` to make the 
intent clearer. What do you think?`;

// Example 4: Edge Case
const feedback4 = `Solid implementation! One edge case to consider: what 
happens if `items` is an empty array? The current code might throw 
an error. Consider adding a guard:

```typescript
if (items.length === 0) {
  return { total: 0, average: 0 };
}
```

Thoughts?`;

// ✅ Feedback Priority Levels

/**
 * Priority Levels for Code Review Feedback
 */

enum FeedbackPriority {
  // Must fix before merging
  BLOCKING = "BLOCKING",
  
  // Should fix, but can be in follow-up PR
  IMPORTANT = "IMPORTANT",
  
  // Nice to have, optional
  SUGGESTION = "SUGGESTION",
  
  // Question or discussion point
  QUESTION = "QUESTION"
}

// ✅ BLOCKING: Security issues, bugs, breaking changes
const blockingFeedback = {
  type: FeedbackPriority.BLOCKING,
  message: "This SQL query is vulnerable to injection attacks. " +
           "Please use parameterized queries before merging.",
  example: `// ❌ VULNERABLE
const query = `SELECT * FROM users WHERE id = ${userId}`;

// ✅ SAFE
const query = 'SELECT * FROM users WHERE id = $1';
await db.query(query, [userId]);`
};

// ✅ IMPORTANT: Code quality, maintainability
const importantFeedback = {
  type: FeedbackPriority.IMPORTANT,
  message: "This function is doing too much. Consider splitting it " +
           "into smaller functions for better testability. Can be " +
           "addressed in a follow-up PR if needed.",
  followUp: true
};

// ✅ SUGGESTION: Style, minor improvements
const suggestionFeedback = {
  type: FeedbackPriority.SUGGESTION,
  message: "Consider using optional chaining here: `user?.profile?.name` " +
           "instead of nested ternaries. Not required, just a style preference.",
  optional: true
};

// ✅ QUESTION: Understanding, discussion
const questionFeedback = {
  type: FeedbackPriority.QUESTION,
  message: "I'm curious about this approach. What was your reasoning " +
           "for using a recursive function here? I'm wondering if an " +
           "iterative approach might be simpler.",
  discussion: true
};

// ✅ GOOD: Use appropriate priority in review tools
function leaveReview(pr: PullRequest, feedback: Feedback[]) {
  const blocking = feedback.filter(f => f.priority === FeedbackPriority.BLOCKING);
  const nonBlocking = feedback.filter(f => f.priority !== FeedbackPriority.BLOCKING);
  
  if (blocking.length > 0) {
    // Use "Request changes" or "Needs work"
    pr.requestChanges(blocking);
  } else if (nonBlocking.length > 0) {
    // Use "Approve with suggestions" or "Approve"
    pr.approveWithComments(nonBlocking);
  } else {
    pr.approve();
  }
}

// ✅ Handling Review Disagreements

/**
 * When you disagree with feedback or approach
 */

// ✅ GOOD: Acknowledge and explain
const response1 = `Thanks for the feedback! I understand your concern about 
performance. I chose this approach because:

1. The data set is small (< 100 items), so the performance difference 
   is negligible
2. This approach is more readable and maintainable
3. We can optimize later if needed when we have real performance data

Would you like to discuss this further, or should we proceed with 
this approach and add a TODO to revisit if performance becomes an issue?`;

// ✅ GOOD: Ask for clarification
const response2 = `I appreciate the suggestion! I'm not sure I understand 
the concern. Could you help me understand what specific issue you're 
seeing? I'd like to make sure I address the right problem.`;

// ✅ GOOD: Propose alternatives
const response3 = `Great point! I see the issue now. What do you think 
about this alternative approach? It addresses your concern while 
keeping the code simple:

[proposed solution]

Does this work better?`;

// ✅ GOOD: Agree to disagree (with escalation)
const response4 = `I understand your perspective, but I have concerns 
about this approach because [reasons]. 

Since we have different views, should we:
1. Discuss this in a team meeting to get more perspectives?
2. Create a small spike to test both approaches?
3. Document the decision and revisit after we have more data?

What do you think?`;

// ❌ BAD: Dismissive responses
// "I disagree"
// "This is fine"
// "You're wrong"
// "I'll change it but I don't agree"

// ✅ GOOD: Escalate when needed
// If there's a fundamental disagreement about architecture or approach,
// involve the team lead or have a design review meeting

// ✅ Architecture Review Checklist

/**
 * Architecture Review Questions
 */

const ARCHITECTURE_CHECKLIST = {
  // Design Patterns
  designPatterns: [
    "Is the chosen design pattern appropriate for this problem?",
    "Are patterns used consistently across the codebase?",
    "Are patterns over-engineered for the problem size?",
    "Do patterns align with team conventions?",
  ],
  
  // Separation of Concerns
  separationOfConcerns: [
    "Are responsibilities clearly separated?",
    "Is there tight coupling that should be loosened?",
    "Are boundaries between layers/modules clear?",
    "Is business logic separated from infrastructure?",
  ],
  
  // Scalability
  scalability: [
    "Will this scale to expected load?",
    "Are there bottlenecks that will become problems?",
    "Is the data model appropriate for growth?",
    "Can this be extended without major refactoring?",
  ],
  
  // Maintainability
  maintainability: [
    "Is the code easy to understand?",
    "Will new team members be able to work with this?",
    "Is the complexity justified?",
    "Are there clear extension points?",
  ],
  
  // Integration
  integration: [
    "Does this integrate well with existing systems?",
    "Are there breaking changes?",
    "Is backward compatibility maintained?",
    "Are dependencies reasonable?",
  ],
  
  // Testing
  testability: [
    "Is the code testable?",
    "Are dependencies injectable?",
    "Can components be tested in isolation?",
    "Are there clear interfaces for mocking?",
  ]
};

// ✅ Example: Reviewing a new service
function reviewServiceArchitecture(service: Service) {
  const concerns = [];
  
  // Check separation of concerns
  if (service.hasBusinessLogic() && service.hasDatabaseAccess()) {
    concerns.push({
      type: "separation",
      message: "Service mixes business logic with data access. " +
               "Consider extracting data access to a repository layer.",
      example: `// Current: Service does everything
class UserService {
  async getUser(id: string) {
    const user = await db.query('SELECT * FROM users WHERE id = ?', [id]);
    return this.transformUser(user);
  }
}

// Better: Separate concerns
class UserRepository {
  async findById(id: string) {
    return db.query('SELECT * FROM users WHERE id = ?', [id]);
  }
}

class UserService {
  constructor(private repo: UserRepository) {}
  
  async getUser(id: string) {
    const user = await this.repo.findById(id);
    return this.transformUser(user);
  }
}`
    });
  }
  
  // Check testability
  if (!service.hasDependencyInjection()) {
    concerns.push({
      type: "testability",
      message: "Service has hard dependencies, making it difficult to test. " +
               "Consider dependency injection.",
    });
  }
  
  return concerns;
}

// ✅ Reviewing Design Pattern Usage

/**
 * Common Pattern Review Scenarios
 */

// Example 1: Factory Pattern
// ✅ GOOD: Factory when you need complex object creation
class PaymentProcessorFactory {
  create(type: PaymentType): PaymentProcessor {
    switch (type) {
      case PaymentType.CREDIT_CARD:
        return new CreditCardProcessor();
      case PaymentType.PAYPAL:
        return new PayPalProcessor();
      case PaymentType.STRIPE:
        return new StripeProcessor();
      default:
        throw new Error(`Unknown payment type: ${type}`);
    }
  }
}

// ❌ BAD: Factory for simple object creation
class UserFactory {
  create(name: string, email: string): User {
    return new User(name, email); // Over-engineered
  }
}

// Example 2: Strategy Pattern
// ✅ GOOD: Strategy when behavior varies
interface DiscountStrategy {
  calculate(price: number): number;
}

class RegularDiscount implements DiscountStrategy {
  calculate(price: number) { return price * 0.9; }
}

class VIPDiscount implements DiscountStrategy {
  calculate(price: number) { return price * 0.7; }
}

class PricingService {
  constructor(private strategy: DiscountStrategy) {}
  
  calculatePrice(basePrice: number) {
    return this.strategy.calculate(basePrice);
  }
}

// ❌ BAD: Strategy for single behavior
class SimpleCalculator {
  constructor(private strategy: AddStrategy) {} // Only one strategy
  // Should just have an add method
}

// Example 3: Observer Pattern
// ✅ GOOD: Observer for event-driven systems
class EventEmitter {
  private listeners = new Map<string, Function[]>();
  
  on(event: string, callback: Function) {
    if (!this.listeners.has(event)) {
      this.listeners.set(event, []);
    }
    this.listeners.get(event)!.push(callback);
  }
  
  emit(event: string, data: any) {
    const callbacks = this.listeners.get(event) || [];
    callbacks.forEach(cb => cb(data));
  }
}

// ❌ BAD: Observer for simple callbacks
class SimpleService {
  private callback?: Function;
  
  setCallback(cb: Function) {
    this.callback = cb; // Just use a callback, not observer pattern
  }
}

// Review Questions:
// 1. Is the pattern solving a real problem?
// 2. Is the complexity justified?
// 3. Will this pattern make the code easier to maintain?
// 4. Is the pattern used consistently?
// 5. Are there simpler alternatives?

// ✅ API Design Review Checklist

/**
 * API Design Review Points
 */

// 1. Naming Consistency
// ✅ GOOD: Consistent naming
GET /api/users/:id
GET /api/users/:id/orders
POST /api/users/:id/orders

// ❌ BAD: Inconsistent naming
GET /api/user/:id
GET /api/users/:id/getOrders
POST /api/createOrder

// 2. HTTP Methods
// ✅ GOOD: Proper HTTP methods
GET /api/users          // List
GET /api/users/:id      // Get one
POST /api/users         // Create
PUT /api/users/:id      // Update (full)
PATCH /api/users/:id    // Update (partial)
DELETE /api/users/:id   // Delete

// ❌ BAD: Using POST for everything
POST /api/getUsers
POST /api/updateUser
POST /api/deleteUser

// 3. Response Format
// ✅ GOOD: Consistent response format
interface ApiResponse<T> {
  data: T;
  meta?: {
    page?: number;
    total?: number;
  };
  errors?: ApiError[];
}

// ❌ BAD: Inconsistent formats
// Sometimes: { user: {...} }
// Sometimes: { data: {...} }
// Sometimes: {...} directly

// 4. Error Handling
// ✅ GOOD: Consistent error format
interface ApiError {
  code: string;
  message: string;
  field?: string;
}

// Status codes:
// 400: Bad Request (validation errors)
// 401: Unauthorized
// 403: Forbidden
// 404: Not Found
// 500: Internal Server Error

// ❌ BAD: Always 200 with error in body
{
  status: 200,
  body: { error: "User not found" }
}

// 5. Versioning
// ✅ GOOD: API versioning
/api/v1/users
/api/v2/users

// ❌ BAD: No versioning strategy
// Makes breaking changes difficult

// 6. Pagination
// ✅ GOOD: Consistent pagination
GET /api/users?page=1&limit=20
{
  data: [...],
  meta: {
    page: 1,
    limit: 20,
    total: 100,
    totalPages: 5
  }
}

// ❌ BAD: No pagination or inconsistent
GET /api/users?offset=0&count=20
GET /api/users?skip=0&take=20

// ✅ Authentication & Authorization Review

/**
 * Security Checklist: Auth
 */

// ✅ GOOD: Proper authentication check
async function requireAuth(req: Request, res: Response, next: NextFunction) {
  const token = req.headers.authorization?.replace('Bearer ', '');
  
  if (!token) {
    return res.status(401).json({ error: 'Unauthorized' });
  }
  
  try {
    const user = await verifyToken(token);
    req.user = user;
    next();
  } catch (error) {
    return res.status(401).json({ error: 'Invalid token' });
  }
}

// ✅ GOOD: Role-based authorization
async function requireRole(role: string) {
  return async (req: Request, res: Response, next: NextFunction) => {
    if (!req.user) {
      return res.status(401).json({ error: 'Unauthorized' });
    }
    
    if (req.user.role !== role && req.user.role !== 'admin') {
      return res.status(403).json({ error: 'Forbidden' });
    }
    
    next();
  };
}

// ✅ GOOD: Resource-level authorization
async function canAccessOrder(req: Request, res: Response, next: NextFunction) {
  const orderId = req.params.id;
  const order = await Order.findById(orderId);
  
  // Check if user owns the resource or is admin
  if (order.userId !== req.user.id && req.user.role !== 'admin') {
    return res.status(403).json({ error: 'Forbidden' });
  }
  
  req.order = order;
  next();
}

// ✅ GOOD: Using middleware
app.get('/api/orders/:id', 
  requireAuth,
  canAccessOrder,
  async (req, res) => {
    res.json(req.order);
  }
);

// ❌ BAD: Missing auth checks
app.get('/api/admin/users', async (req, res) => {
  // No authentication check!
  const users = await User.findAll();
  res.json(users);
});

// ❌ BAD: Client-side only authorization
// Frontend: if (user.role === 'admin') { showAdminPanel(); }
// Backend: No check - anyone can call the API

// Security Review Questions:
// 1. Is authentication required for all protected endpoints?
// 2. Is authorization checked (not just authentication)?
// 3. Are resource-level permissions verified?
// 4. Are admin endpoints properly protected?
// 5. Is sensitive data filtered based on user permissions?
// 6. Are tokens properly validated and not expired?
// 7. Is password hashing using secure algorithms (bcrypt, argon2)?

// ✅ XSS and Injection Prevention

/**
 * XSS Prevention in React
 */

// ✅ GOOD: React auto-escapes by default
function UserComment({ comment }: { comment: string }) {
  // Safe: React escapes HTML automatically
  return <div>{comment}</div>;
}

// ✅ GOOD: Sanitize if HTML is needed
import DOMPurify from 'dompurify';

function RichComment({ html }: { html: string }) {
  const sanitized = DOMPurify.sanitize(html, {
    ALLOWED_TAGS: ['b', 'i', 'em', 'strong', 'a', 'p'],
    ALLOWED_ATTR: ['href']
  });
  return <div dangerouslySetInnerHTML={{ __html: sanitized }} />;
}

// ❌ BAD: Unsanitized HTML
function DangerousComment({ html }: { html: string }) {
  return <div dangerouslySetInnerHTML={{ __html: html }} />;
  // Vulnerable to XSS!
}

/**
 * SQL Injection Prevention
 */

// ✅ GOOD: Parameterized queries
async function getUser(id: string) {
  return db.query('SELECT * FROM users WHERE id = ?', [id]);
}

// ✅ GOOD: Using ORM (prevents injection)
async function getUser(id: string) {
  return User.findOne({ where: { id } });
}

// ❌ BAD: String concatenation
async function getUser(id: string) {
  return db.query(`SELECT * FROM users WHERE id = '${id}'`);
  // Vulnerable to SQL injection!
}

/**
 * Command Injection Prevention
 */

// ✅ GOOD: Validate and sanitize
import { exec } from 'child_process';
import { promisify } from 'util';

const execAsync = promisify(exec);

async function runCommand(command: string) {
  // Whitelist allowed commands
  const allowedCommands = ['ls', 'pwd', 'date'];
  const [cmd, ...args] = command.split(' ');
  
  if (!allowedCommands.includes(cmd)) {
    throw new Error('Command not allowed');
  }
  
  // Validate arguments
  for (const arg of args) {
    if (!/^[a-zA-Z0-9._-]+$/.test(arg)) {
      throw new Error('Invalid characters in arguments');
    }
  }
  
  return execAsync(command);
}

// ❌ BAD: Direct execution
async function runCommand(command: string) {
  return execAsync(command); // Vulnerable to command injection!
}

/**
 * NoSQL Injection Prevention
 */

// ✅ GOOD: Parameterized queries
async function findUser(username: string) {
  return User.findOne({ 
    username: username // MongoDB driver handles this safely
  });
}

// ❌ BAD: String interpolation in queries
async function findUser(username: string) {
  return db.collection('users').find({
    $where: `this.username === "${username}"`
  }); // Vulnerable to NoSQL injection!
}

// Security Review Checklist:
// [ ] All user input is validated
// [ ] HTML is sanitized before rendering
// [ ] SQL queries use parameterized statements
// [ ] No eval() or similar dangerous functions
// [ ] Command execution is restricted and validated
// [ ] NoSQL queries use safe methods
// [ ] File uploads are validated and scanned
// [ ] URLs are validated before use