Code Reviewer

Code Reviewer Skill

Overview

Comprehensive code review skill implementing systematic checklist-based reviews covering organization, error handling, performance, security, test coverage, and code quality standards.

Capabilities

1. Architecture & Design Review

• Design patterns validation
• SOLID principles compliance
• Separation of concerns
• Dependency management
• Module coupling analysis

2. Security Review

• OWASP Top 10 compliance
• Input validation
• Authentication/authorization
• Sensitive data handling
• Dependency vulnerabilities

3. Performance Review

• Algorithm complexity (Big O)
• Database query optimization (N+1)
• Caching strategies
• Resource management
• Memory leaks

4. Error Handling Review

• Exception handling patterns
• Logging strategies
• Error messages
• Graceful degradation
• Rollback mechanisms

5. Test Coverage Review

• Unit test presence
• Integration test coverage
• Edge case handling
• Test quality assessment
• Mocking appropriateness

6. Code Quality Review

• Code style compliance
• Documentation completeness
• Naming conventions
• Code duplication (DRY)
• Complexity metrics

Code Review Checklist

1. Organization & Structure

✓ Files organized logically
✓ Clear module boundaries
✓ Appropriate layer separation (presentation/business/data)
✓ No circular dependencies
✓ Configuration externalized
✓ Constants properly defined

Example Review:

# ❌ BAD: Mixed concerns
class UserController:
    def create_user(self, data):
        # Validation (should be in validator)
        if not data.get('email'):
            return {'error': 'Email required'}

        # Business logic (should be in service)
        user = User(email=data['email'])

        # Database access (should be in repository)
        db.session.add(user)
        db.session.commit()

        return {'success': True}

# ✅ GOOD: Separated concerns
class UserController:
    def __init__(self, user_service, validator):
        self.user_service = user_service
        self.validator = validator

    def create_user(self, data):
        errors = self.validator.validate(data)
        if errors:
            return {'errors': errors}, 400

        user = self.user_service.create(data)
        return {'user': user.to_dict()}, 201

2. Error Handling

✓ Exceptions caught at appropriate levels
✓ Specific exception types used
✓ Error messages are informative
✓ Logging includes context
✓ Resources properly released (try-finally, context managers)
✓ No empty catch blocks

Example Review:

// ❌ BAD: Generic exception, no cleanup
public void processFile(String filename) throws Exception {
    FileReader reader = new FileReader(filename);
    // ... processing ...
    reader.close();  // Won't execute if exception occurs!
}

// ✅ GOOD: Specific exception, automatic cleanup
public void processFile(String filename) throws IOException {
    try (FileReader reader = new FileReader(filename)) {
        // ... processing ...
        logger.info("Processed file: {}", filename);
    } catch (FileNotFoundException e) {
        logger.error("File not found: {}", filename, e);
        throw new ApplicationException("Cannot process missing file", e);
    } catch (IOException e) {
        logger.error("Error reading file: {}", filename, e);
        throw new ApplicationException("File processing failed", e);
    }
}

3. Performance

✓ No N+1 query problems
✓ Appropriate indexing
✓ Efficient algorithms (no O(n²) where O(n) possible)
✓ Connection pooling configured
✓ Batch operations where applicable
✓ Caching for expensive operations
✓ No premature optimization

Example Review:

# ❌ BAD: N+1 queries
def get_users_with_orders():
    users = User.query.all()  # 1 query
    result = []
    for user in users:
        orders = user.orders.all()  # N queries!
        result.append({
            'user': user,
            'orders': orders
        })
    return result

# ✅ GOOD: Eager loading
def get_users_with_orders():
    users = User.query.options(
        joinedload(User.orders)
    ).all()  # 1 query with JOIN
    return [{'user': u, 'orders': u.orders} for u in users]

4. Security

✓ Input validated and sanitized
✓ SQL injection prevented (parameterized queries)
✓ XSS prevented (output encoding)
✓ Authentication/authorization checked
✓ Sensitive data encrypted
✓ No hardcoded secrets
✓ HTTPS enforced
✓ CSRF protection enabled

Example Review:

// ❌ BAD: SQL injection, XSS, hardcoded secret
app.get('/user/:id', (req, res) => {
  const apiKey = 'sk-12345';  // Hardcoded!
  const query = `SELECT * FROM users WHERE id = ${req.params.id}`;  // SQL injection!
  db.query(query, (err, result) => {
    res.send(`<div>User: ${result.name}</div>`);  // XSS!
  });
});

// ✅ GOOD: Secure implementation
app.get('/user/:id', authenticate, authorize('read:users'), (req, res) => {
  const userId = parseInt(req.params.id, 10);
  if (isNaN(userId)) {
    return res.status(400).json({ error: 'Invalid user ID' });
  }

  const query = 'SELECT * FROM users WHERE id = ?';  // Parameterized
  db.query(query, [userId], (err, result) => {
    if (err) {
      logger.error('Database error', err);
      return res.status(500).json({ error: 'Internal error' });
    }
    res.json(sanitize(result));  // Sanitized output
  });
});

5. Test Coverage

✓ Unit tests for business logic
✓ Integration tests for APIs
✓ Edge cases tested
✓ Error paths tested
✓ Mock external dependencies
✓ Test coverage > 80%
✓ Tests are fast (< 1 minute)

Example Review:

# ❌ BAD: Incomplete tests
def test_divide():
    assert divide(10, 2) == 5  # Only happy path!

# ✅ GOOD: Comprehensive tests
def test_divide_positive_numbers():
    assert divide(10, 2) == 5

def test_divide_negative_numbers():
    assert divide(-10, 2) == -5

def test_divide_by_zero_raises_error():
    with pytest.raises(ZeroDivisionError):
        divide(10, 0)

def test_divide_float_result():
    assert divide(5, 2) == 2.5

@pytest.mark.parametrize("a,b,expected", [
    (10, 2, 5),
    (9, 3, 3),
    (7, 2, 3.5),
])
def test_divide_parameterized(a, b, expected):
    assert divide(a, b) == expected

6. Code Quality

✓ Follows style guide (PEP 8, Airbnb, Google)
✓ Functions < 50 lines
✓ Classes have single responsibility
✓ No code duplication
✓ Meaningful variable names
✓ Comments explain "why", not "what"
✓ Cyclomatic complexity < 10

Example Review:

// ❌ BAD: Too long, multiple responsibilities, unclear names
public void p(String s) {
    // Parse and validate
    String[] parts = s.split(",");
    if (parts.length != 3) throw new Exception("Bad");
    int x = Integer.parseInt(parts[0]);
    int y = Integer.parseInt(parts[1]);
    String z = parts[2];

    // Business logic
    if (x < 0 || y < 0) throw new Exception("Negative");
    int result = x * y;

    // Database operation
    Connection conn = DriverManager.getConnection(url);
    PreparedStatement stmt = conn.prepareStatement("INSERT INTO t VALUES (?, ?)");
    stmt.setInt(1, result);
    stmt.setString(2, z);
    stmt.execute();
    conn.close();

    // Logging
    System.out.println("Done: " + result);
}

// ✅ GOOD: Separated, clear, testable
public class OrderProcessor {
    private final OrderRepository repository;
    private final Logger logger;

    public void processOrder(OrderRequest request) {
        validateOrder(request);
        Order order = calculateOrder(request);
        repository.save(order);
        logger.info("Order processed: {}", order.getId());
    }

    private void validateOrder(OrderRequest request) {
        if (request.getQuantity() <= 0 || request.getPrice() <= 0) {
            throw new InvalidOrderException("Quantity and price must be positive");
        }
    }

    private Order calculateOrder(OrderRequest request) {
        int total = request.getQuantity() * request.getPrice();
        return new Order(total, request.getProductName());
    }
}

Integration Scripts

code_review.sh

Automated code review tool:

#!/bin/bash
# Comprehensive automated code review

PROJECT_DIR=${1:-.}
REPORT_FILE="code-review-$(date +%Y%m%d-%H%M%S).md"

echo "# Code Review Report" > $REPORT_FILE
echo "Date: $(date)" >> $REPORT_FILE
echo "" >> $REPORT_FILE

# 1. Code style
echo "## Code Style" >> $REPORT_FILE
if command -v pylint &> /dev/null; then
    echo "Running Python linting..."
    pylint $PROJECT_DIR --output-format=text >> $REPORT_FILE 2>&1
fi

if command -v eslint &> /dev/null; then
    echo "Running JavaScript linting..."
    eslint $PROJECT_DIR >> $REPORT_FILE 2>&1
fi

# 2. Security scan
echo "" >> $REPORT_FILE
echo "## Security Issues" >> $REPORT_FILE
if command -v semgrep &> /dev/null; then
    echo "Running security scan..."
    semgrep --config=auto --json $PROJECT_DIR | \
      jq -r '.results[] | "- [\(.check_id)] \(.extra.message) at \(.path):\(.start.line)"' >> $REPORT_FILE
fi

# 3. Test coverage
echo "" >> $REPORT_FILE
echo "## Test Coverage" >> $REPORT_FILE
if [ -f "pytest.ini" ]; then
    pytest --cov --cov-report=term-missing | tail -20 >> $REPORT_FILE
elif [ -f "package.json" ]; then
    npm test -- --coverage --silent | tail -20 >> $REPORT_FILE
fi

# 4. Complexity analysis
echo "" >> $REPORT_FILE
echo "## Complexity" >> $REPORT_FILE
if command -v radon &> /dev/null; then
    radon cc $PROJECT_DIR -a -nc >> $REPORT_FILE
fi

echo "Code review report generated: $REPORT_FILE"

review_checklist.py

Interactive code review checklist:

#!/usr/bin/env python3
"""Interactive code review checklist"""

CHECKLIST = {
    "Architecture": [
        "Design patterns appropriate",
        "SOLID principles followed",
        "Separation of concerns",
        "No circular dependencies",
    ],
    "Security": [
        "Input validation present",
        "SQL injection prevented",
        "XSS prevented",
        "Authentication/authorization checked",
        "No hardcoded secrets",
    ],
    "Performance": [
        "No N+1 queries",
        "Efficient algorithms",
        "Appropriate caching",
        "Connection pooling configured",
    ],
    "Error Handling": [
        "Exceptions properly caught",
        "Logging includes context",
        "Resources properly released",
        "Error messages informative",
    ],
    "Testing": [
        "Unit tests present",
        "Edge cases covered",
        "Error paths tested",
        "Coverage > 80%",
    ],
    "Code Quality": [
        "Style guide followed",
        "Functions < 50 lines",
        "No code duplication",
        "Clear naming",
        "Appropriate comments",
    ],
}

def run_review():
    print("=== Code Review Checklist ===\n")

    results = {}
    for category, items in CHECKLIST.items():
        print(f"\n{category}:")
        results[category] = []

        for item in items:
            while True:
                response = input(f"  ✓ {item}? (y/n/skip): ").lower()
                if response in ['y', 'n', 'skip', 's']:
                    if response == 'y':
                        results[category].append((item, True))
                    elif response == 'n':
                        results[category].append((item, False))
                        note = input("    Note: ")
                        results[category].append((item, False, note))
                    break

    # Print summary
    print("\n=== Review Summary ===\n")
    total = 0
    passed = 0

    for category, items in results.items():
        category_passed = sum(1 for item in items if len(item) == 2 and item[1])
        category_total = len(items)
        total += category_total
        passed += category_passed

        print(f"{category}: {category_passed}/{category_total}")

        # Show failed items
        failed = [item for item in items if len(item) >= 2 and not item[1]]
        if failed:
            for item in failed:
                print(f"  ✗ {item[0]}")
                if len(item) > 2:
                    print(f"    → {item[2]}")

    percentage = (passed / total * 100) if total > 0 else 0
    print(f"\nOverall: {passed}/{total} ({percentage:.1f}%)")

    if percentage >= 90:
        print("✅ APPROVED: Excellent code quality")
    elif percentage >= 70:
        print("⚠️  CHANGES REQUESTED: Address issues above")
    else:
        print("❌ REJECTED: Major issues need resolution")

if __name__ == '__main__':
    run_review()

complexity_checker.py

Check code complexity:

#!/usr/bin/env python3
import os
import sys

def check_complexity(directory, threshold=10):
    """Check cyclomatic complexity of Python files"""
    try:
        import radon.complexity as cc
        from radon.cli import Config
    except ImportError:
        print("Install radon: pip install radon")
        sys.exit(1)

    issues = []

    for root, dirs, files in os.walk(directory):
        for file in files:
            if file.endswith('.py'):
                filepath = os.path.join(root, file)
                with open(filepath) as f:
                    results = cc.cc_visit(f.read())

                for result in results:
                    if result.complexity > threshold:
                        issues.append({
                            'file': filepath,
                            'function': result.name,
                            'complexity': result.complexity,
                            'line': result.lineno
                        })

    if issues:
        print(f"⚠️  Found {len(issues)} functions with complexity > {threshold}:\n")
        for issue in sorted(issues, key=lambda x: x['complexity'], reverse=True):
            print(f"  {issue['file']}:{issue['line']}")
            print(f"    {issue['function']}: complexity = {issue['complexity']}")
            print(f"    💡 Consider refactoring into smaller functions\n")
        sys.exit(1)
    else:
        print(f"✅ All functions have complexity ≤ {threshold}")
        sys.exit(0)

if __name__ == '__main__':
    directory = sys.argv[1] if len(sys.argv) > 1 else '.'
    check_complexity(directory)

Pull Request Review Template

## Code Review

### Summary
Brief description of changes...

### Checklist

#### Architecture & Design
- [ ] Design patterns appropriate
- [ ] SOLID principles followed
- [ ] Clear separation of concerns
- [ ] No circular dependencies

#### Security
- [ ] Input validated
- [ ] No SQL injection
- [ ] No XSS vulnerabilities
- [ ] Authentication/authorization present
- [ ] No hardcoded secrets

#### Performance
- [ ] No N+1 queries
- [ ] Efficient algorithms
- [ ] Appropriate caching
- [ ] Database indexes present

#### Error Handling
- [ ] Exceptions properly handled
- [ ] Logging includes context
- [ ] Resources released properly
- [ ] Error messages informative

#### Testing
- [ ] Unit tests added/updated
- [ ] Edge cases covered
- [ ] Error paths tested
- [ ] Coverage maintained/improved

#### Code Quality
- [ ] Style guide followed
- [ ] Functions reasonably sized
- [ ] No code duplication
- [ ] Clear naming
- [ ] Appropriate documentation

### Comments

**Architecture**:
- Consider using Repository pattern for database access

**Security**:
- Add input validation for email field

**Performance**:
- Use eager loading for user.orders relationship

**Testing**:
- Add test for error case when user not found

### Decision
- [ ] ✅ Approved
- [ ] 💬 Comment
- [ ] ⚠️  Request Changes

Best Practices

1. Review Small Changes: Easier to review < 400 lines
2. Use Checklists: Ensure consistent reviews
3. Automate: Use linters, formatters, security scanners
4. Be Constructive: Explain why, suggest alternatives
5. Focus on Important: Don't nitpick style if linter exists
6. Test: Actually run the code if possible
7. Security First: Always check for security issues
8. Performance: Watch for N+1, inefficient algorithms
9. Documentation: Verify complex logic is documented
10. Follow Up: Track that feedback is addressed

Requirements

# Python
pip install pylint radon bandit

# JavaScript
npm install -g eslint

# Security
pip install semgrep

# Complexity
pip install radon mccabe

Metrics to Track

• Review time: < 1 hour per review
• Issues found: Track categories
• Time to fix: Days to address feedback
• Re-review rate: % requiring second review
• Code quality trend: Improving over time
• Security issues: Trending down