Code Review Analysis

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

Instructions

1. 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

2. 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
}

3. 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 });
});

4. 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]

5. 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

6. 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)

Review Feedback Template

## Code Review: [PR Title]

### Summary
Brief overview of changes and overall assessment.

### ✅ Strengths
- Well-structured error handling
- Comprehensive test coverage
- Clear documentation

### 🔍 Issues Found

#### 🔴 Critical (Must Fix)
1. **Security**: SQL injection vulnerability in user query (line 45)
   ```python
   # Current code
   query = f"SELECT * FROM users WHERE id = '{user_id}'"

   # Suggested fix
   query = "SELECT * FROM users WHERE id = ?"
   cursor.execute(query, (user_id,))

🟡 Moderate (Should Fix)

1. Performance: N+1 query problem (lines 78-82)
   • Suggest using eager loading to reduce database queries

🟢 Minor (Consider)

1. Style: Consider extracting this function for better testability
2. Naming: proc_data could be more descriptive as processUserData

💡 Suggestions

• Consider adding input validation
• Could benefit from additional edge case tests
• Documentation could include usage examples

📋 Checklist

• Security vulnerabilities addressed
• Tests added and passing
• Documentation updated
• No console.log or debug statements
• Error handling is appropriate

Verdict

✅ Approved with minor suggestions | ⏸️ Needs changes | ❌ Needs major revision

## Common Issues Checklist

### Security
- [ ] No SQL injection vulnerabilities
- [ ] XSS prevention in place
- [ ] CSRF protection where needed
- [ ] Authentication/authorization checks
- [ ] No exposed secrets or credentials
- [ ] Input validation implemented
- [ ] Output encoding applied

### Code Quality
- [ ] Functions are focused and small
- [ ] Names are descriptive
- [ ] No code duplication
- [ ] Appropriate comments
- [ ] Consistent style
- [ ] No magic numbers
- [ ] Error messages are helpful

### Performance
- [ ] No N+1 queries
- [ ] Appropriate indexing
- [ ] Efficient algorithms
- [ ] No unnecessary computations
- [ ] Proper caching where beneficial
- [ ] Resource cleanup

### Testing
- [ ] Tests included for new code
- [ ] Edge cases covered
- [ ] Error cases tested
- [ ] Integration tests if needed
- [ ] Tests are maintainable
- [ ] No flaky tests

### Maintainability
- [ ] Code is self-documenting
- [ ] Complex logic is explained
- [ ] No premature optimization
- [ ] Follows SOLID principles
- [ ] Dependencies are appropriate
- [ ] Backwards compatibility considered

## Tools

- **Linters**: ESLint, Pylint, RuboCop
- **Security**: Snyk, OWASP Dependency Check, Bandit
- **Code Quality**: SonarQube, Code Climate
- **Coverage**: Istanbul, Coverage.py
- **Static Analysis**: TypeScript, Flow, mypy

## 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

## Examples

See the refactor-legacy-code skill for detailed refactoring examples that often apply during code review.