Code Review Practices

Review Mindset

• Catch bugs and issues early
• Share knowledge across the team
• Maintain code quality standards
• Improve code maintainability
• Foster learning and growth

• Assert dominance or superiority
• Enforce personal preferences
• Block progress unnecessarily
• Nitpick without value

What to Review

1. Correctness

• Logic is sound
• Edge cases are handled
• Error conditions are managed
• Requirements are met

2. Design and Architecture

• Follows existing patterns
• Appropriate abstractions
• Reasonable complexity
• Scalable approach

3. Readability

• Clear naming
• Logical organization
• Appropriate comments
• Consistent style

4. Testing

• Tests exist for new code
• Edge cases covered
• Tests are maintainable
• Integration points tested

5. Security

• Input validation
• Authentication/authorization
• Sensitive data handling
• Known vulnerabilities

6. Performance

• No obvious bottlenecks
• Efficient algorithms
• Resource usage reasonable
• Scalability considered

Review Process

1. Understand Context

• Read PR description and linked issues
• Understand the problem being solved
• Check CI/CD results
• Note any special considerations

2. Review at Appropriate Level

• Overall approach
• Architecture decisions
• Major design choices

• Implementation specifics
• Edge cases
• Code style

3. Provide Actionable Feedback

• Specific and clear
• Explains the "why"
• Suggests solutions
• Prioritizes issues

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

• "This will cause a race condition when..."
• "Security issue: User input is not sanitized..."
• "This breaks the API contract by..."

• "Consider: This could be simplified by..."
• "Nit: Variable name could be more descriptive"
• "Question: Why did you choose X over Y?"

• "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

• Bugs and logic errors
• Security vulnerabilities
• Performance issues
• Breaking changes

• Code style choices
• Naming preferences
• Organization approaches
• Minor optimizations

Communication Guidelines

Be Constructive

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

• "This could cause X issue because..."
• "Have you considered Y approach? It might..."
• "This could be improved by..."

Explain Your Reasoning

This should be refactored.

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

• "What happens if X is null here?"
• "Could this be simplified using Y?"
• "Have you considered the case where...?"

Acknowledge Good Work

• "Clever use of X to solve Y"
• "Great test coverage"
• "This is much cleaner than the old approach"
• "Nice documentation"

Be Humble

• "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

• Hard to understand
• Difficult to test
• Multiple responsibilities

• Maintenance burden
• Inconsistent behavior
• Missed bug fixes

• Hard to reason about
• Error-prone
• Difficult to test

• Too many responsibilities
• Hard to maintain
• Tight coupling

• 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

• Code style and formatting
• Naming conventions
• Comment requirements
• Test coverage expectations
• Documentation needs

• Style guides
• Architecture decision records
• Review checklists
• Example code

Review 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

• State your opinion
• Explain your reasoning
• Accept the author's choice
• Move on

• Clearly explain the issue
• Provide evidence or examples
• Suggest alternatives
• Escalate if needed

When Author Disagrees

• They may have context you lack
• There might be constraints you don't know
• Your suggestion might not fit

• Explain your concerns
• Ask questions
• Find common ground
• Compromise when appropriate

Review Timing

Response Time

• Small PRs: Within 4 hours
• Medium PRs: Within 1 day
• Large PRs: Within 2 days

• Let the author know
• Suggest another reviewer
• Set expectations

Review Size

• 200-400 lines of code
• Focused on single concern
• Reviewable in 30-60 minutes

• Request breakdown if possible
• Review in multiple passes
• Focus on high-level first
• Consider pair review

Mentoring Through Reviews

Teaching Opportunities

• Explain patterns and practices
• Share relevant resources
• Demonstrate better approaches
• Encourage questions

Growing Reviewers

• Invite junior developers to review
• Explain your review comments
• Discuss review decisions
• Share review best practices

Anti-Patterns to Avoid

Review Outcomes

Approve

• All critical issues addressed
• Tests pass
• Documentation updated
• No blocking concerns

Approve with Comments

• Style suggestions
• Future improvements
• Questions for discussion
• Nice-to-have changes

Request Changes

• Bugs or logic errors
• Security vulnerabilities
• Missing tests
• Breaking changes
• Architectural concerns

Reject (Rare)

• Wrong approach entirely
• Doesn't solve the problem
• Creates major technical debt
• Violates core principles

Quick Reference

1. Identify the issue
2. Explain why it's a problem
3. Suggest a solution
4. Indicate severity (blocking vs suggestion)

[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.

• 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