Code Review

When to use this skill

• Reviewing pull requests
• Checking code quality
• Providing feedback on implementations
• Identifying potential bugs
• Suggesting improvements
• Security audits
• Performance analysis

Instructions

Step 1: Understand the context

Read the PR description:

• What is the goal of this change?
• Which issues does it address?
• Are there any special considerations?

Check the scope:

• How many files changed?
• What type of changes? (feature, bugfix, refactor)
• Are tests included?

Step 2: High-level review

Architecture and design:

• Does the approach make sense?
• Is it consistent with existing patterns?
• Are there simpler alternatives?
• Is the code in the right place?

Code organization:

• Clear separation of concerns?
• Appropriate abstraction levels?
• Logical file/folder structure?

Step 3: Detailed code review

Naming:

• Variables: descriptive, meaningful names
• Functions: verb-based, clear purpose
• Classes: noun-based, single responsibility
• Constants: UPPER_CASE for true constants
• Avoid abbreviations unless widely known

Functions:

• Single responsibility
• Reasonable length (< 50 lines ideally)
• Clear inputs and outputs
• Minimal side effects
• Proper error handling

Classes and objects:

• Single responsibility principle
• Open/closed principle
• Liskov substitution principle
• Interface segregation
• Dependency inversion

Error handling:

• All errors caught and handled
• Meaningful error messages
• Proper logging
• No silent failures
• User-friendly errors for UI

Code quality:

• No code duplication (DRY)
• No dead code
• No commented-out code
• No magic numbers
• Consistent formatting

Step 4: Security review

Input validation:

• All user inputs validated
• Type checking
• Range checking
• Format validation

Authentication & Authorization:

• Proper authentication checks
• Authorization for sensitive operations
• Session management
• Password handling (hashing, salting)

Data protection:

• No hardcoded secrets
• Sensitive data encrypted
• SQL injection prevention
• XSS prevention
• CSRF protection

Dependencies:

• No vulnerable packages
• Dependencies up-to-date
• Minimal dependency usage

Step 5: Performance review

Algorithms:

• Appropriate algorithm choice
• Reasonable time complexity
• Reasonable space complexity
• No unnecessary loops

Database:

• Efficient queries
• Proper indexing
• N+1 query prevention
• Connection pooling

Caching:

• Appropriate caching strategy
• Cache invalidation handled
• Memory usage reasonable

Resource management:

• Files properly closed
• Connections released
• Memory leaks prevented

Step 6: Testing review

Test coverage:

• Unit tests for new code
• Integration tests if needed
• Edge cases covered
• Error cases tested

Test quality:

• Tests are readable
• Tests are maintainable
• Tests are deterministic
• No test interdependencies
• Proper test data setup/teardown

Test naming:

# Good
def test_user_creation_with_valid_data_succeeds():
    pass

# Bad
def test1():
    pass

Step 7: Documentation review

Code comments:

• Complex logic explained
• No obvious comments
• TODOs have tickets
• Comments are accurate

Function documentation:

def calculate_total(items: List[Item], tax_rate: float) -> Decimal:
    """
    Calculate the total price including tax.

    Args:
        items: List of items to calculate total for
        tax_rate: Tax rate as decimal (e.g., 0.1 for 10%)

    Returns:
        Total price including tax

    Raises:
        ValueError: If tax_rate is negative
    """
    pass

README/docs:

• README updated if needed
• API docs updated
• Migration guide if breaking changes

Step 8: Provide feedback

Be constructive:

✅ Good:
"Consider extracting this logic into a separate function for better
testability and reusability:

def validate_email(email: str) -> bool:
    return '@' in email and '.' in email.split('@')[1]

This would make it easier to test and reuse across the codebase."

❌ Bad:
"This is wrong. Rewrite it."

Be specific:

✅ Good:
"On line 45, this query could cause N+1 problem. Consider using
.select_related('author') to fetch related objects in a single query."

❌ Bad:
"Performance issues here."

Prioritize issues:

• 🔴 Critical: Security, data loss, major bugs
• 🟡 Important: Performance, maintainability
• 🟢 Nice-to-have: Style, minor improvements

Acknowledge good work:

"Nice use of the strategy pattern here! This makes it easy to add
new payment methods in the future."

Review checklist

Functionality

• Code does what it's supposed to do
• Edge cases handled
• Error cases handled
• No obvious bugs

Code Quality

• Clear, descriptive naming
• Functions are small and focused
• No code duplication
• Consistent with codebase style
• No code smells

Security

• Input validation
• No hardcoded secrets
• Authentication/authorization
• No SQL injection vulnerabilities
• No XSS vulnerabilities

Performance

• No obvious bottlenecks
• Efficient algorithms
• Proper database queries
• Resource management

Testing

• Tests included
• Good test coverage
• Tests are maintainable
• Edge cases tested

Documentation

• Code is self-documenting
• Comments where needed
• Docs updated
• Breaking changes documented

Common issues

Anti-patterns

God class:

# Bad: One class doing everything
class UserManager:
    def create_user(self): pass
    def send_email(self): pass
    def process_payment(self): pass
    def generate_report(self): pass

Magic numbers:

# Bad
if user.age > 18:
    pass

# Good
MINIMUM_AGE = 18
if user.age > MINIMUM_AGE:
    pass

Deep nesting:

# Bad
if condition1:
    if condition2:
        if condition3:
            if condition4:
                # deeply nested code

# Good (early returns)
if not condition1:
    return
if not condition2:
    return
if not condition3:
    return
if not condition4:
    return
# flat code

Security vulnerabilities

SQL Injection:

# Bad
query = f"SELECT * FROM users WHERE id = {user_id}"

# Good
query = "SELECT * FROM users WHERE id = %s"
cursor.execute(query, (user_id,))

XSS:

// Bad
element.innerHTML = userInput;

// Good
element.textContent = userInput;

Hardcoded secrets:

# Bad
API_KEY = "sk-1234567890abcdef"

# Good
API_KEY = os.environ.get("API_KEY")

Best practices

1. Review promptly: Don't make authors wait
2. Be respectful: Focus on code, not the person