Article 11: Code Review Practices

Introduction

Code review is one of the most valuable practices in software engineering. Through participating in thousands of reviews—both as author and reviewer—I've learned that effective code review isn't about finding bugs; it's about sharing knowledge, maintaining quality, and building better software together.

This article covers how to conduct meaningful code reviews, write reviewable pull requests, and foster a healthy review culture.

Why Code Review Matters

Benefits

Benefit	Description
Bug Prevention	Catch issues before they reach production
Knowledge Sharing	Spread understanding across the team
Code Quality	Maintain consistent standards
Mentorship	Learn from each other's approaches
Documentation	PRs become historical records

Writing Reviewable Pull Requests

Keep PRs Small

Guidelines:

• Target 200-400 lines of changes

• One logical change per PR

• Split large features into smaller PRs

• Separate refactoring from new features

Write Descriptive PR Titles

# Bad titles
- "Fix bug"
- "Update code"
- "WIP"

# Good titles
- "Fix: User authentication fails with expired tokens"
- "Add: Rate limiting to API endpoints"
- "Refactor: Extract payment processing into service class"

PR Description Template

## Summary
Brief description of what this PR does.

## Motivation
Why is this change needed? Link to issue if applicable.

Closes #123

## Changes
- Added `UserService` class for user management
- Updated authentication middleware
- Added unit tests for new functionality

## Testing
- [ ] Unit tests pass
- [ ] Integration tests pass
- [ ] Manual testing completed

## Screenshots (if applicable)
[Add screenshots for UI changes]

## Checklist
- [ ] Code follows project style guidelines
- [ ] Self-reviewed the code
- [ ] Added/updated tests
- [ ] Updated documentation
- [ ] No breaking changes (or documented if any)

Self-Review First

Before requesting review:

# Before submitting, ask yourself:

# 1. Does the code work?
def process_order(order: Order) -> Result:
    # Have I tested this locally?
    pass

# 2. Is the code clear?
# Would a teammate understand this without asking me?

# 3. Are there any debug artifacts?
# print("DEBUG: got here")  # Remove these!
# import pdb; pdb.set_trace()  # Remove these!

# 4. Are tests included?
def test_process_order_success():
    # Every new feature needs tests
    pass

# 5. Is the commit history clean?
# Did I squash or organize commits logically?

Conducting Effective Reviews

The Review Mindset

# Think like a maintainer who will work with this code in 6 months

class ReviewerMindset:
    """Approach every review with these principles."""
    
    def __init__(self):
        self.goals = [
            "Understand the change",
            "Ensure correctness",
            "Maintain quality",
            "Help the author improve",
        ]
    
    def review(self, pr: PullRequest) -> None:
        # Start with understanding
        self.read_description()
        self.understand_context()
        
        # Then evaluate
        self.check_correctness()
        self.check_readability()
        self.check_maintainability()
        
        # Finally, provide feedback
        self.give_constructive_feedback()

What to Look For

Review Checklist

## Correctness
- [ ] Logic is correct
- [ ] Edge cases are handled
- [ ] Error conditions are managed
- [ ] No obvious bugs

## Design
- [ ] Approach is reasonable
- [ ] Code is in the right place
- [ ] Follows existing patterns
- [ ] Not over-engineered

## Quality
- [ ] Code is readable
- [ ] Naming is clear
- [ ] No unnecessary complexity
- [ ] Comments where needed

## Testing
- [ ] Tests are included
- [ ] Tests are meaningful
- [ ] Tests cover edge cases
- [ ] Tests are maintainable

## Security
- [ ] No sensitive data exposed
- [ ] Input is validated
- [ ] Authentication/authorization correct

Giving Feedback

Be Kind and Constructive

# ❌ Bad feedback
"This is wrong."
"Why would you do it this way?"
"This doesn't make sense."

# ✅ Good feedback
"Consider using a dictionary here for O(1) lookup instead of a list."
"I find this logic a bit hard to follow. Could we extract it into a named function?"
"I think there might be an edge case when the list is empty. What do you think?"

Use Prefixes for Clarity

# Blocking issues (must fix)
[BLOCKER] This will cause a null pointer exception when user is None.

# Suggestions (take it or leave it)
[NIT] Consider renaming this to `calculate_total` for clarity.
[OPTIONAL] You could use a list comprehension here.

# Questions (seeking understanding)
[QUESTION] What happens if the API call times out here?

# Praise (positive reinforcement)
[NICE] Great use of the strategy pattern here!

Provide Context and Alternatives

# ❌ Bad: Just saying it's wrong
# "This is inefficient."

# ✅ Good: Explain why and suggest alternatives
# "This has O(n²) complexity because of the nested loop. 
#  Consider using a set for O(1) lookups:
#
#  existing_ids = {user.id for user in users}
#  new_users = [u for u in candidates if u.id not in existing_ids]
#
#  This would reduce complexity to O(n)."

Review Response Time

Quick reviews keep momentum and prevent context switching.

Responding to Feedback

Accept Feedback Gracefully

class AuthorMindset:
    """How to receive and respond to review feedback."""
    
    def receive_feedback(self, comment: Comment) -> None:
        # Remember: reviewers are trying to help
        
        # Don't take it personally
        self.separate_code_from_self()
        
        # Understand the feedback
        if not self.understand(comment):
            self.ask_for_clarification()
        
        # Respond constructively
        if self.agree(comment):
            self.make_change()
            self.thank_reviewer()
        else:
            self.explain_reasoning()
            self.discuss_alternatives()

Responding to Comments

# Acknowledge and fix
> Consider using a context manager here for better resource handling.

Thanks, good catch! Updated to use `with` statement. ✓

# Explain if you disagree
> This could be simplified to a one-liner.

I kept it explicit because the one-liner version is harder to debug 
when the transformation fails. Happy to discuss if you feel strongly!

# Ask for clarification
> The error handling seems incomplete.

Could you clarify which cases you're concerned about? 
I'm handling ValueError and TypeError - are there others to consider?

PR Workflow

Standard Review Flow

GitHub PR Best Practices

# .github/PULL_REQUEST_TEMPLATE.md
## Description
<!-- What does this PR do? -->

## Related Issues
<!-- Link related issues: Closes #123 -->

## Type of Change
- [ ] Bug fix
- [ ] New feature
- [ ] Breaking change
- [ ] Documentation update

## Testing
- [ ] Unit tests pass
- [ ] Integration tests pass
- [ ] Manual testing completed

## Checklist
- [ ] Self-reviewed code
- [ ] Updated documentation
- [ ] Added tests
- [ ] No linting errors

Branch Protection Rules

# Recommended branch protection settings for main:
protection_rules:
  require_pull_request:
    required_approving_reviews: 1
    dismiss_stale_reviews: true
    require_code_owner_reviews: true
  
  require_status_checks:
    checks:
      - lint
      - test
    require_branches_up_to_date: true
  
  require_conversation_resolution: true
  require_signed_commits: false
  restrict_pushes: true

CODEOWNERS

# .github/CODEOWNERS

# Default owners for everything
* @team-lead

# Specific paths
/src/auth/           @security-team
/src/api/            @api-team
/infrastructure/     @devops-team
/docs/               @docs-team

# Specific files
pyproject.toml       @team-lead @devops-team
.github/workflows/   @devops-team

Review Culture

Building a Healthy Review Culture

class HealthyReviewCulture:
    """Principles for a productive review environment."""
    
    principles = [
        "Reviews are about code, not people",
        "Everyone's code gets reviewed",
        "Feedback is a gift",
        "Questions are welcome",
        "Mistakes are learning opportunities",
    ]
    
    anti_patterns = [
        "Nitpicking to show off",
        "Approving without reading",
        "Personal attacks",
        "Blocking for style preferences",
        "Avoiding giving feedback",
    ]

Common Review Anti-Patterns

Anti-Pattern	Problem	Solution
Rubber Stamping	Approving without reviewing	Take time to understand changes
Nitpicking	Blocking on minor style issues	Use linters for style, focus on logic
Ghost Reviewer	Never responding to requests	Set review time expectations
Perfectionism	Demanding perfect code	Accept "good enough" solutions
Scope Creep	Requesting unrelated changes	Open separate issues for improvements

Handling Disagreements

def resolve_disagreement(author_view: str, reviewer_view: str) -> Resolution:
    """Process for resolving review disagreements."""
    
    # Step 1: Ensure mutual understanding
    clarify_both_positions()
    
    # Step 2: Focus on the code, not preferences
    evaluate_against_principles()
    
    # Step 3: Consider the impact
    if is_minor_issue():
        return defer_to_author()
    
    # Step 4: Get help if needed
    if still_disagreed():
        return get_third_opinion()
    
    # Step 5: Document the decision
    record_decision_and_rationale()

Practical Exercises

Exercise 1: Review This Code

Practice reviewing the following code:

# Code to review
def process(d):
    r = []
    for i in d:
        if i['status'] == 'active':
            r.append({'id': i['id'], 'name': i['name'].upper()})
    return r

Sample Review Feedback:

Several suggestions to improve readability and maintainability:

1. [BLOCKER] Parameter `d` and variable `r` are not descriptive. 
   Consider `users` and `active_users`.

2. [NIT] The function name `process` doesn't describe what it does.
   Consider `get_active_users_formatted`.

3. [OPTIONAL] Could be simplified with a list comprehension:
   ```python
   def get_active_users(users: list[dict]) -> list[dict]:
       return [
           {"id": user["id"], "name": user["name"].upper()}
           for user in users
           if user["status"] == "active"
       ]

1. [QUESTION] Should we handle the case where 'status', 'id', or 'name' keys are missing?

### Exercise 2: Write a PR Description

Write a PR description for adding a caching feature:

```markdown
## Summary
Add Redis-based caching for user profile lookups to reduce database load.

## Motivation
User profile queries account for 40% of database traffic. 
By caching profiles for 5 minutes, we can reduce database load 
and improve response times.

Closes #456

## Changes
- Added `CacheService` class with Redis backend
- Integrated caching into `UserRepository.get_by_id()`
- Added cache invalidation on profile updates
- Added cache metrics to monitoring

## Testing
- Unit tests for CacheService
- Integration tests with Redis container
- Load tested with 1000 concurrent requests

## Performance Impact
- Response time: 150ms → 15ms (cached)
- Database queries: -40%
- Memory usage: +50MB (cache size)

## Rollback Plan
Feature is behind `ENABLE_CACHE` flag. Set to `false` to disable.

Key Takeaways

1. Keep PRs small - Easier to review, easier to merge

2. Write clear descriptions - Help reviewers understand context

3. Be kind in feedback - Focus on code, not people

4. Use prefixes - Clarify blocking vs. optional feedback

5. Respond promptly - Keep momentum, respect others' time

6. Self-review first - Catch obvious issues before requesting review

What's Next?

With effective code review practices in place, you're ready to ship your software. In Article 12: Production Readiness, we'll cover containerization with Docker, configuration management, and monitoring basics.

---

This article is part of the Software Engineering 101 series.