Code Review Analysis

Code Review Analysis

Table of Contents

• Overview
• When to Use
• Quick Start
• Reference Guides
• Best Practices

Overview

Systematic code review process covering code quality, security, performance, maintainability, and best practices following industry standards.

When to Use

• Reviewing pull requests and merge requests
• Analyzing code quality before merging
• Identifying security vulnerabilities
• Providing constructive feedback to developers
• Ensuring coding standards compliance
• Mentoring through code review

Quick Start

Minimal working example:

# Check the changes
git diff main...feature-branch

# Review file changes
git diff --stat main...feature-branch

# Check commit history
git log main...feature-branch --oneline

Reference Guides

Detailed implementations in the references/ directory:

Best Practices

✅ DO

• Be constructive and respectful
• Explain the "why" behind suggestions
• Provide code examples
• Ask questions if unclear
• Acknowledge good practices
• Focus on important issues
• Consider the context
• Offer to pair program on complex issues

❌ DON'T

• Be overly critical or personal
• Nitpick minor style issues (use automated tools)
• Block on subjective preferences
• Review too many changes at once (>400 lines)
• Forget to check tests
• Ignore security implications
• Rush the review

Best Practices

Best Practices

Error Handling

// ❌ Silent failures
try {
  await saveData(data);
} catch (e) {
  // empty catch
}

// ✅ Proper error handling
try {
  await saveData(data);
} catch (error) {
  logger.error("Failed to save data", { error, data });
  throw new DataSaveError("Could not save data", { cause: error });
}

Resource Management

# ❌ Resources not closed
file = open('data.txt')
data = file.read()
process(data)

# ✅ Proper cleanup
with open('data.txt') as file:
    data = file.read()
    process(data)

Code Quality Analysis

Code Quality Analysis

Readability

# ❌ Poor readability
def p(u,o):
    return u['t']*o['q'] if u['s']=='a' else 0

# ✅ Good readability
def calculate_order_total(user: User, order: Order) -> float:
    """Calculate order total with user-specific pricing."""
    if user.status == 'active':
        return user.tier_price * order.quantity
    return 0

Complexity

// ❌ High cognitive complexity
function processData(data) {
  if (data) {
    if (data.type === "user") {
      if (data.status === "active") {
        if (data.permissions && data.permissions.length > 0) {
          // deeply nested logic
        }
      }
    }
  }
}

// ✅ Reduced complexity with early returns
function processData(data) {
  if (!data) return null;
  if (data.type !== "user") return null;
  if (data.status !== "active") return null;
  if (!data.permissions?.length) return null;

  // main logic at top level
}

Initial Assessment

Initial Assessment

# Check the changes
git diff main...feature-branch

# Review file changes
git diff --stat main...feature-branch

# Check commit history
git log main...feature-branch --oneline

Quick Checklist:

• [ ] PR description is clear and complete
• [ ] Changes match the stated purpose
• [ ] No unrelated changes included
• [ ] Tests are included
• [ ] Documentation is updated

Performance Review

Performance Review

// ❌ N+1 query problem
const users = await User.findAll();
for (const user of users) {
  user.orders = await Order.findAll({ where: { userId: user.id } });
}

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

# ❌ Inefficient list operations
result = []
for item in large_list:
    if item % 2 == 0:
        result.append(item * 2)

# ✅ List comprehension
result = [item * 2 for item in large_list if item % 2 == 0]

Security Review

Security Review

Common Vulnerabilities

SQL Injection

# ❌ Vulnerable to SQL injection
query = f"SELECT * FROM users WHERE email = '{user_email}'"

# ✅ Parameterized query
query = "SELECT * FROM users WHERE email = ?"
cursor.execute(query, (user_email,))

XSS Prevention

// ❌ XSS vulnerable
element.innerHTML = userInput;

// ✅ Safe rendering
element.textContent = userInput;
// or use framework escaping: {{ userInput }} in templates

Authentication & Authorization

// ❌ Missing authorization check
app.delete("/api/users/:id", async (req, res) => {
  await deleteUser(req.params.id);
  res.json({ success: true });
});

// ✅ Proper authorization
app.delete("/api/users/:id", requireAuth, async (req, res) => {
  if (req.user.id !== req.params.id && !req.user.isAdmin) {
    return res.status(403).json({ error: "Forbidden" });
  }
  await deleteUser(req.params.id);
  res.json({ success: true });
});

Testing Review

Testing Review

Test Coverage

describe("User Service", () => {
  // ✅ Tests edge cases
  it("should handle empty input", () => {
    expect(processUser(null)).toBeNull();
  });

  it("should handle invalid data", () => {
    expect(() => processUser({})).toThrow(ValidationError);
  });

  // ✅ Tests happy path
  it("should process valid user", () => {
    const result = processUser(validUserData);
    expect(result.id).toBeDefined();
  });
});

Check for:

• [ ] Unit tests for new functions
• [ ] Integration tests for new features
• [ ] Edge cases covered
• [ ] Error cases tested
• [ ] Mock/stub usage is appropriate