Code Review Best Practices: Effective Feedback and Collaboration

**Situation**: "In the user authentication function..."
**Evidence**: "I noticed the password is stored using MD5 hash..."
**Value**: "This is a security concern because MD5 is considered broken..."

// ❌ Bad: Vague criticism
"This code is bad."

// ✅ Good: Specific with evidence
"The try-catch block on line 45 catches Exception, which is too broad. 
Consider catching specific exceptions like FileNotFoundException and 
IOException to handle each case appropriately."

// ❌ Bad: Demanding tone
"Change this to use a map."

// ✅ Good: Suggestive with reasoning
"Consider using a HashMap here for O(1) lookups instead of iterating 
through the list each time. This would improve performance from O(n) to O(1)."

# ❌ Aggressive
"You forgot to validate the input. This is a bug."

# ✅ Collaborative
"Should we add input validation here to handle edge cases?"

# ❌ Dismissive  
"This logic is wrong."

# ✅ Educational
"The current logic assumes X, but consider the case where Y. 
Here's an example of how this could cause an issue..."

# ❌ Vague
"This could be better."

# ✅ Actionable
"Extract this into a separate function for better readability.
Consider calling it `calculateTotalWithTax()`"

# Mindset checklist for receiving feedback

def on_receive_feedback(feedback):
    # 1. Pause before reacting
    pause_and_breathe()
    
    # 2. Separate identity from code
    remember("I am not my code")
    
    # 3. Consider the source
    # The reviewer wants to help, not criticize
    
    # 4. Ask clarifying questions
    if feedback.is_vague():
        ask_for_specifics()
    
    # 5. Thank the reviewer
    express_gratitude()
    
    # 6. Decide on action
    if feedback.is_valid():
        implement_fix()
    else:
        explain_reason_not_to()

class FeedbackHandler:
    
    def __init__(self):
        self.common_triggers = {
            "nitpick": self.handle_nitpick,
            "major_criticism": self.handle_criticism,
            "style_change": self.handle_style,
            "suggestion": self.handle_suggestion
        }
    
    def handle_nitpick(self, feedback):
        """Minor issues - usually just fix it."""
        if is_quick_fix(feedback):
            return make_fix()
        return discuss_tradeoffs(feedback)
    
    def handle_criticism(self, feedback):
        """Major concerns - take seriously."""
        # Ask clarifying questions
        # Consider the impact
        # Discuss in person if needed
        return evaluate_and_respond()
    
    def handle_style(self, feedback):
        """Style suggestions - use linter instead."""
        # Suggest automating with linter/formatter
        # Reduces future debates
        return propose_automation()
    
    def handle_suggestion(self, feedback):
        """Suggestions - consider but don't have to accept."""
        # Evaluate tradeoffs
        # Accept if improvement
        # Politely decline if not beneficial
        return evaluate_and_respond()

## Code Review Checklist

### Functionality
- [ ] Does the code do what it's supposed to?
- [ ] Are edge cases handled?
- [ ] Are there any obvious bugs?
- [ ] Will this work in production?

### Security
- [ ] Any security vulnerabilities?
- [ ] Sensitive data properly protected?
- [ ] Input validation in place?
- [ ] Authentication/authorization correct?

### Performance
- [ ] Any performance concerns?
- [ ] Database queries optimized?
- [ ] Memory usage acceptable?
- [ ] Algorithmic complexity reasonable?

### Testing
- [ ] Are tests included?
- [ ] Tests cover happy path and edge cases?
- [ ] Tests are readable and maintainable?

### Code Quality
- [ ] Follows style guide?
- [ ] Code is readable?
- [ ] Functions are reasonably sized?
- [ ] No duplication?

### Documentation
- [ ] Comments explain why, not what?
- [ ] Public APIs documented?
- [ ] README updated if needed?

def review_efficiently(pull_request):
    # 1. Understand the context
    read_description()
    check_linked_issues()
    
    # 2. Run the code locally
    checkout_branch()
    run_tests()
    
    # 3. Review in logical order
    # Start with: Architecture > Logic > Style
    
    # 4. Use tools to help
    run_linter()
    run_static_analysis()
    check_test_coverage()
    
    # 5. Focus on important issues
    # Don't nitpick style (that's what formatters are for)
    
    # 6. Group related comments
    # Don't comment on every line - group by issue
    
    return review_notes()

| PR Size | Expected Review Time | Turnaround Target |
|---------|---------------------|-------------------|
| < 100 lines | 10-15 minutes | Same day |
| 100-400 lines | 30-60 minutes | Same day |
| 400+ lines | Consider splitting | Within 2 days |

## Best Practices

### For Authors

1. **Keep PRs Small**

```python
# ❌ Large PR - hard to review
# 2000 lines changed, 15 files

# ✅ Small PR - easy to review
# 200 lines changed, 2 files, 1 feature

## Summary
Implements user authentication using JWT tokens.

## Changes
- Added JWT validation middleware
- Created /login and /logout endpoints
- Added token refresh logic

## Testing
- Unit tests added for authentication service
- Manual testing completed in staging

## Screenshots (if UI)
N/A

# Before requesting review, check:
def before_review_request():
    run_all_tests()          # Do tests pass?
    run_linter()            # Any style issues?
    check_coverage()        # Test coverage acceptable?
    read_diff_as_reviewer()  # Does it make sense?
    fix_obvious_issues()   # Don't waste reviewer time
    
    return ready_for_review()

# When you receive feedback:
def on_feedback_received(feedback):
    # Acknowledge within 24 hours
    if time_since_feedback > 24 * 60 * 60:
        send_acknowledgment()
    
    # Address within reasonable time
    if feedback.blocks_merge():
        prioritize_fix()

# Don't let PRs stagnate
def on_receive_pr():
    # Initial review within 24 hours
    # Complete review within 48 hours
    
    if cannot_review_now():
        reassign_to_another()
        or
        provide_initial_feedback("Looks good, will do detailed review tomorrow")

# ❌ Vague
"This is wrong."

# ✅ Specific
"Line 45: The null check here doesn't handle the case where 
`user.preferences` exists but is an empty object. 
Should we add `Object.keys(user.preferences).length > 0`?"

# ❌ Just what
"Use const here."

# ✅ What and why
"Use const instead of let here. Since this value is never 
reassigned after initialization, const better communicates 
intent and enables certain JavaScript optimizations."

## Code Review Template

### Overview
[Summary of what this PR does]

### Strengths
- [ ] 
- [ ]

### Questions/Comments
- [ ] 
- [ ]

### Required Changes
- [ ] 
- [ ]

### Nitpicks (optional)
- [ ]

# CODE_REVIEW_STANDARDS.md

## Review Response Time
- Initial response: 24 hours
- Complete review: 48 hours
- Urgent fixes: 4 hours

## What Requires 2 Approvals
- Security changes
- API changes
- Database migrations
- New dependencies

## What Can Be Nitpick-Free
- Test files
- Documentation
- Refactoring (if tests pass)

## Code Review Etiquette
1. Be kind but direct
2. Focus on the code, not the person
3. Explain the "why" behind suggestions
4. Accept that you might be wrong
5. Use questions instead of commands

# .github/workflows/automated-checks.yml
name: Automated Checks

on: [pull_request]

jobs:
  lint:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
      - name: Run linter
        run: npm run lint
      
  test:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
      - name: Run tests
        run: npm test
      
  security:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
      - name: Security audit
        run: npm audit
      
  maintainability:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
      - name: SonarCloud
        run: npm run sonar

def scale_review_process(team_size):
    if team_size < 5:
        return {
            "reviewers": "Anyone can review",
            "approvals_needed": 1,
            "template": "simple"
        }
    
    elif team_size < 20:
        return {
            "reviewers": "Domain experts + anyone",
            "approvals_needed": 2,
            "template": "standard",
            "automation": ["lint", "test", "security"]
        }
    
    else:
        return {
            "reviewers": "Required reviewers + optional",
            "approvals_needed": 2,
            "template": "detailed",
            "automation": ["lint", "test", "security", "coverage"],
            "review_rotation": True,
            "senior_review_required": ["security", "infrastructure"]
        }

# ❌ Anti-patterns

1. **The Nitpicker**
Commenting on every minor style issue instead of focusing on what matters.

2. **The Ghost**
Never responding to review requests or taking weeks to review.

3. **The Approver**
Rubber-stamping everything without actually reviewing.

4. **The Blocker**
Blocking merges on trivial issues or personal preferences.

5. **The Vague**
Leaving comments like "This doesn't seem right" without explanation.

# ❌ Anti-patterns

1. **The Defensive**
Getting defensive about any criticism.

2. **The Silent**
Ignoring feedback and not responding.

3. **The Large PR**
Sending massive PRs that are impossible to review properly.

4. **The Surprise**
Including changes that aren't in the description.

5. **The Dependent**
Batching multiple unrelated changes together.

# Metrics to track

review_metrics = {
    "time_to_first_response": "hours",
    "time_to_approval": "hours",
    "comments_per_review": "count",
    "revision_requests": "count",
    "reviewer_load": "distribution"
}

# Questions to ask
- Are we reviewing quickly enough?
- Are the same issues coming up repeatedly?
- Is one person doing all the reviews?
- Are reviews blocking deployments?
- Is code quality improving?