Code Review Practices

This skill provides guidance for conducting effective, constructive code reviews that improve code quality and team collaboration.

Review Mindset

Goals of code review:

• Catch bugs and issues early

• Share knowledge across the team

• Maintain code quality standards

• Improve code maintainability

• Foster learning and growth

Not goals:

• Assert dominance or superiority

• Enforce personal preferences

• Block progress unnecessarily

• Nitpick without value

What to Review

1. Correctness

Does the code work as intended?

• Logic is sound

• Edge cases are handled

• Error conditions are managed

• Requirements are met

2. Design and Architecture

Does it fit the system?

• Follows existing patterns

• Appropriate abstractions

• Reasonable complexity

• Scalable approach

3. Readability

Can others understand it?

• Clear naming

• Logical organization

• Appropriate comments

• Consistent style

4. Testing

Is it adequately tested?

• Tests exist for new code

• Edge cases covered

• Tests are maintainable

• Integration points tested

5. Security

Are there security concerns?

• Input validation

• Authentication/authorization

• Sensitive data handling

• Known vulnerabilities

6. Performance

Will it perform well?

• No obvious bottlenecks

• Efficient algorithms

• Resource usage reasonable

• Scalability considered

Review Process

1. Understand Context

Before reviewing code:

• Read PR description and linked issues

• Understand the problem being solved

• Check CI/CD results

• Note any special considerations

2. Review at Appropriate Level

High-level first:

• Overall approach

• Architecture decisions

• Major design choices

Then details:

• Implementation specifics

• Edge cases

• Code style

3. Provide Actionable Feedback

Good feedback:

• Specific and clear

• Explains the "why"

• Suggests solutions

• Prioritizes issues

Example:

Consider using a Set instead of an array here for O(1) lookups. With the current implementation, the nested loop creates O(n²) complexity which could be problematic with large datasets.

4. Use Review Comments Effectively

Types of comments:

Blocking (must fix):

• "This will cause a race condition when..."

• "Security issue: User input is not sanitized..."

• "This breaks the API contract by..."

Non-blocking (suggestions):

• "Consider: This could be simplified by..."

• "Nit: Variable name could be more descriptive"

• "Question: Why did you choose X over Y?"

Positive:

• "Nice solution! This is much cleaner than..."

• "Great test coverage on the edge cases"

• "I learned something new from this approach"

5. Distinguish Preferences from Problems

Problems (objective):

• Bugs and logic errors

• Security vulnerabilities

• Performance issues

• Breaking changes

Preferences (subjective):

• Code style choices

• Naming preferences

• Organization approaches

• Minor optimizations

Rule: Block on problems, discuss preferences.

Communication Guidelines

Be Constructive

Instead of:

• "This is wrong"

• "Why would you do it this way?"

• "This code is terrible"

Say:

• "This could cause X issue because..."

• "Have you considered Y approach? It might..."

• "This could be improved by..."

Explain Your Reasoning

Weak comment:

This should be refactored.

Strong comment:

Consider extracting this logic into a separate method. It's used in three places and would be easier to test and maintain as a standalone function.

Ask Questions

Use questions to understand and guide:

• "What happens if X is null here?"

• "Could this be simplified using Y?"

• "Have you considered the case where...?"

Acknowledge Good Work

Don't only point out problems:

• "Clever use of X to solve Y"

• "Great test coverage"

• "This is much cleaner than the old approach"

• "Nice documentation"

Be Humble

You might be wrong:

• "I might be missing something, but..."

• "Could you explain why...?"

• "I'm not familiar with X, but..."

Common Review Patterns

Code Smells to Watch For

Long methods/functions:

• Hard to understand

• Difficult to test

• Multiple responsibilities

Duplicated code:

• Maintenance burden

• Inconsistent behavior

• Missed bug fixes

Complex conditionals:

• Hard to reason about

• Error-prone

• Difficult to test

Large classes:

• Too many responsibilities

• Hard to maintain

• Tight coupling

Magic numbers/strings:

• Unclear meaning

• Hard to change

• Error-prone

Security Red Flags

• Hardcoded credentials or secrets

• SQL injection vulnerabilities

• XSS vulnerabilities

• Missing input validation

• Insecure data storage

• Weak authentication

• Missing authorization checks

Performance Concerns

• N+1 query problems

• Inefficient algorithms (O(n²) when O(n) possible)

• Memory leaks

• Unnecessary database calls

• Large data structures in memory

• Blocking operations in async code

Review Standards

Establish Team Norms

Define standards for:

• Code style and formatting

• Naming conventions

• Comment requirements

• Test coverage expectations

• Documentation needs

Document standards:

• Style guides

• Architecture decision records

• Review checklists

• Example code

Review Checklist

Use a consistent checklist:

• Code is correct and handles edge cases

• Tests exist and pass

• No security vulnerabilities

• Performance is acceptable

• Code is readable and maintainable

• Documentation is updated

• No breaking changes (or properly communicated)

• Follows team conventions

Handling Disagreements

When You Disagree

If it's a preference:

• State your opinion

• Explain your reasoning

• Accept the author's choice

• Move on

If it's a problem:

• Clearly explain the issue

• Provide evidence or examples

• Suggest alternatives

• Escalate if needed

When Author Disagrees

Listen and understand:

• They may have context you lack

• There might be constraints you don't know

• Your suggestion might not fit

Discuss, don't dictate:

• Explain your concerns

• Ask questions

• Find common ground

• Compromise when appropriate

Review Timing

Response Time

Aim for:

• Small PRs: Within 4 hours

• Medium PRs: Within 1 day

• Large PRs: Within 2 days

If you can't review promptly:

• Let the author know

• Suggest another reviewer

• Set expectations

Review Size

Optimal PR size:

• 200-400 lines of code

• Focused on single concern

• Reviewable in 30-60 minutes

For large PRs:

• Request breakdown if possible

• Review in multiple passes

• Focus on high-level first

• Consider pair review

Mentoring Through Reviews

Teaching Opportunities

Use reviews to share knowledge:

• Explain patterns and practices

• Share relevant resources

• Demonstrate better approaches

• Encourage questions

Growing Reviewers

Help others become better reviewers:

• Invite junior developers to review

• Explain your review comments

• Discuss review decisions

• Share review best practices

Anti-Patterns to Avoid

❌ Nitpicking without value - Focus on meaningful improvements ❌ Blocking on style preferences - Use automated tools instead ❌ Rewriting in your style - Respect different approaches ❌ Reviewing too quickly - Take time to understand ❌ Being overly critical - Balance criticism with praise ❌ Ignoring context - Consider constraints and tradeoffs ❌ Making it personal - Focus on code, not the person

Review Outcomes

Approve

Code meets standards and is ready to merge:

• All critical issues addressed

• Tests pass

• Documentation updated

• No blocking concerns

Approve with Comments

Minor issues that don't block merge:

• Style suggestions

• Future improvements

• Questions for discussion

• Nice-to-have changes

Request Changes

Issues that must be addressed:

• Bugs or logic errors

• Security vulnerabilities

• Missing tests

• Breaking changes

• Architectural concerns

Reject (Rare)

Fundamental problems requiring redesign:

• Wrong approach entirely

• Doesn't solve the problem

• Creates major technical debt

• Violates core principles

Quick Reference

Good review comment structure:

• Identify the issue

• Explain why it's a problem

• Suggest a solution

• Indicate severity (blocking vs suggestion)

Example:

[Blocking] This query will cause N+1 problem when loading related records. Consider using eager loading with includes() to fetch all data in a single query. This will significantly improve performance with large datasets.

Remember:

• Be kind and constructive

• Focus on the code, not the person

• Explain your reasoning

• Acknowledge good work

• Know when to compromise

• Foster learning and growth