Code Review: Reviewing Code Effectively

Why Code Review?

Code review catches issues before they ship:

• Find bugs early


• Share knowledge


• Maintain standards


• Improve code quality


• Learn from each other

The Reviewer's Role

You're a collaborator, not a gatekeeper:

• Help improve the code


• Share knowledge


• Catch issues


• Approve good work

What to Look For

Correctness

Does it work?

# Bug: off by one
for i in range(len(items) - 1):  # Misses last item
    process(items[i])

Logic Errors

Is the logic right?

# Bug: wrong condition
if user.age > 18:  # Should be >= for "18 or older"
    allow_access()

Edge Cases

What about unusual inputs?

# What if items is empty?
first = items[0]  # Crashes!

# Better
first = items[0] if items else None

Security

Any vulnerabilities?

# SQL injection risk
query = f"SELECT * FROM users WHERE id = {user_input}"

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

Performance

Any obvious issues?

# N+1 query problem
for user in users:
    orders = db.query(f"SELECT * FROM orders WHERE user_id = {user.id}")

# Better: batch query
orders = db.query("SELECT * FROM orders WHERE user_id IN (...)")

Readability

Can you understand it?

# Unclear
x = [i for i in d if d[i] > t and i not in e]

# Clear
active_items = [
    item_id 
    for item_id, count in item_counts.items()
    if count > threshold and item_id not in excluded
]

Maintainability

Will this be painful to change?

• Hardcoded values?


• Tight coupling?


• Missing abstractions?

Tests

Is it tested?

• Are there tests?


• Do they cover the changes?


• Do they test edge cases?

Giving Feedback

Be Constructive

❌ "This is wrong"
✅ "This might fail if the list is empty. Consider adding a check."

❌ "Use better names"
✅ "Could 'data' be renamed to 'user_profile' to clarify what it contains?"

Explain Why

❌ "Don't use global variables"
✅ "Global variables make testing harder and can cause unexpected 
    behavior when the code runs concurrently. Consider passing 
    this as a parameter instead."

Distinguish Severity

Blocking:

🔴 "This will crash if user is null - needs a check before proceeding"

Suggestion:

💡 "Consider using a dictionary comprehension here for readability"

Question:

❓ "Why is this timeout set to 60 seconds? Is that intentional?"

Nitpick:

🔍 "Nit: extra blank line here"

Ask Questions

Sometimes you're wrong:

"I don't understand why this is needed. Can you explain?"
"Is there a reason we're not using X here?"

Acknowledge Good Work

"Nice refactor! This is much cleaner."
"Good catch on that edge case."
"I learned something from this approach."

Review Process

First Pass: Understand

Read the PR description

Understand the goal

Get the big picture

Second Pass: Deep Dive

Review file by file

Check logic carefully

Note issues as you go

Third Pass: Summary

Overall assessment

Major concerns

Approval or changes needed

Common Review Mistakes

Being Too Harsh

❌ "This is terrible code"
❌ "Why would you do it this way?"
❌ "Obviously wrong"

✅ "I think there might be a better approach here..."
✅ "Consider..."
✅ "This might cause issues because..."

Nitpicking Everything

Focus on what matters:

Not worth blocking:
- Minor style preferences
- Naming bikeshedding
- "I would have done it differently"

Worth discussing:
- Bugs
- Security issues
- Significant performance problems
- Maintainability concerns

Not Reviewing Thoroughly

Don't just skim:

• Actually read the code


• Think about edge cases


• Consider the context

Blocking on Preferences

❌ "I prefer tabs over spaces" [blocks merge]
✅ "Nit: we usually use tabs, but not blocking"

Receiving Review

Don't Take It Personally

Feedback is about the code, not you.

Respond to All Comments

• Fix issues or explain why not
• Don't leave comments hanging

Ask for Clarification

"I'm not sure I understand - could you elaborate?"
"Can you suggest how you'd approach this?"

Thank Reviewers

They spent time helping you.

Code Review Checklist

Functionality

• [ ] Does it work correctly?
• [ ] Are edge cases handled?
• [ ] Are errors handled properly?

Security

• [ ] Input validation?
• [ ] No injection vulnerabilities?
• [ ] Sensitive data protected?

Quality

• [ ] Readable and clear?
• [ ] No obvious performance issues?
• [ ] Follows project conventions?

Testing

• [ ] Tests included?
• [ ] Tests pass?
• [ ] Coverage adequate?

Documentation

• [ ] Code self-documenting?
• [ ] Complex parts explained?
• [ ] API docs updated if needed?

Reviewing as an Agent

When asked to review code:

## Code Review Summary

### What I Reviewed
Brief description of the changes

### Issues Found
🔴 **Critical:** [issue and fix]
🟡 **Suggestion:** [improvement idea]
🔵 **Nitpick:** [minor style issue]

### What Looks Good
Positive observations

### Recommendation
Approve / Request Changes / Need Clarification

Conclusion

Good code review:

• Finds real issues


• Gives constructive feedback


• Respects the author


• Improves the codebase


• Shares knowledge

Review code like you'd want your code reviewed.

---

Next: Logging Best Practices - Effective logging for debugging