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:

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)

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

🟢 Minor (Consider)

Style: Consider extracting this function for better testability
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.

code-review-analysis