Code Review Basics

Why Code Review Matters

Code review isn't just about finding bugs—it serves multiple crucial purposes:

Benefits of Code Review

• Catch bugs early: Find issues before they reach production
• Improve code quality: Ensure code is readable, maintainable, and follows standards
• Share knowledge: Team members learn from each other
• Spread ownership: More people understand each part of the codebase
• Maintain consistency: Keep code style and patterns uniform
• Prevent technical debt: Address problems before they compound
• Build team culture: Foster collaboration and learning
• Onboard new developers: Reviews help newcomers learn the codebase

The Code Review Process

Here's how code review typically works on GitHub:

Step 1: You're Assigned or Find a PR

• Assigned by maintainers (you get a notification)
• You volunteer to review
• You regularly check for new PRs in projects you follow

Step 2: Initial Assessment

Quickly check:

• PR description: Is it clear what this changes and why?
• Size: Is it small enough to review effectively?
• Related issues: Does it address the stated problem?
• CI status: Are tests passing?

Step 3: Review the Code

Read through the changes carefully (more on what to look for below).

Step 4: Leave Comments

Add comments on specific lines or general feedback.

Step 5: Submit Your Review

Choose one of three actions:

• Comment: Give feedback without formal approval/rejection
• Approve: Code looks good, ready to merge
• Request Changes: Issues must be fixed before merge

Step 6: Follow Up

Continue discussion, review updates, and approve when ready.

How to Review on GitHub

Option 1: Review on GitHub (Most Common)

Starting a Review

1. Go to the Pull Request on GitHub
2. Click the "Files changed" tab
3. You'll see a diff of all changes

Adding Line Comments

1. Hover over a line of code
2. Click the + button that appears
3. Write your comment
4. Click "Start a review" (first comment) or "Add review comment" (subsequent comments)

Adding Suggestions

GitHub lets you suggest specific code changes:

Click the suggestion icon in the comment box, then:

```suggestion
// Your suggested code here
const result = items.map(item => item.value);
```

The author can accept this with one click!

Adding General Comments

For feedback not tied to specific lines:

1. Scroll to the bottom of "Files changed" tab
2. Write your overall feedback in the review summary box

Submitting Your Review

1. Click "Review changes" button (top-right)
2. Write a summary of your review
3. Choose:
   • Comment: Neutral feedback
   • Approve: LGTM (Looks Good To Me)
   • Request changes: Must be addressed
4. Click "Submit review"

Option 2: Review Locally (For Testing)

Sometimes you want to run the code locally:

# Method 1: Fetch PR by number
git fetch origin pull/123/head:pr-123
git checkout pr-123

# Method 2: Add contributor's fork as remote
git remote add contributor https://github.com/contributor/repo.git
git fetch contributor
git checkout contributor/feature-branch

# Test the changes
npm install
npm test
npm start

# When done reviewing locally, switch back
git checkout main

Using GitHub CLI

# Install GitHub CLI first: https://cli.github.com

# Checkout a PR
gh pr checkout 123

# View PR details
gh pr view 123

# Review with comments
gh pr review 123 --comment -b "Great work! Just a few suggestions..."

# Approve
gh pr review 123 --approve

# Request changes
gh pr review 123 --request-changes -b "Please address these issues"

What to Look For During Review

Here's a systematic approach to reviewing code:

1. Correctness

• Does it work? Does the code do what it's supposed to?
• Edge cases: What happens with empty input, null values, large data?
• Logic errors: Are there any logical mistakes?
• Off-by-one errors: Array indices, loop conditions

2. Testing

• Tests included: Are there tests for new features?
• Tests pass: Do existing tests still pass?
• Test coverage: Are important cases tested?
• Test quality: Do tests actually verify behavior?

3. Code Quality

• Readability: Is the code easy to understand?
• Naming: Are variables/functions well-named?
• Complexity: Is it unnecessarily complicated?
• DRY principle: Is there repeated code that could be extracted?
• Functions: Are functions small and focused?

4. Design & Architecture

• Design patterns: Are appropriate patterns used?
• Separation of concerns: Is logic properly separated?
• Dependencies: Are new dependencies justified?
• Scalability: Will this work with more data/users?

5. Security

• Input validation: Is user input validated?
• SQL injection: Are queries parameterized?
• XSS prevention: Is output properly escaped?
• Authentication: Are protected routes actually protected?
• Secrets: No API keys or passwords in code?

6. Performance

• Algorithm efficiency: Could this be faster?
• Database queries: N+1 query problems?
• Memory usage: Are there memory leaks?
• Caching: Should results be cached?

7. Documentation

• Code comments: Are complex sections explained?
• Documentation updated: README, API docs, etc.
• Examples provided: For new features
• Changelog: Is it updated if applicable?

8. Code Style

• Project conventions: Does it match existing code?
• Linting rules: Does it pass the linter?
• Formatting: Consistent indentation, spacing
• Import organization: Consistent import order

Giving Constructive Feedback

How you communicate feedback is as important as what you say.

The Feedback Formula

Good vs Bad Feedback Examples

Example 1: Naming

Example 2: Logic Issue

const processed = items.map(item => ({
  ...item,
  processed: true
}));

Example 3: Performance

Feedback Best Practices

Do:

• Ask questions: "Why did you choose this approach?"
• Explain reasoning: "This could cause X because Y"
• Provide examples: Show what you mean with code
• Acknowledge good work: "Nice solution to the edge case!"
• Be specific: Point to exact lines and issues
• Consider context: Understand time constraints, scope
• Offer to help: "Want to pair on this?"

Don't:

• Be harsh: "This is terrible code"
• Be vague: "Something seems off here"
• Make it personal: "You always do this"
• Demand without explaining: "Change this"
• Bikeshed: Argue about trivial style preferences
• Review while angry: Take a break if frustrated
• Forget to praise: Acknowledge what's done well

Types of Review Comments

1. Blocking Issues (Must Fix)

Critical problems that prevent merging:

• Security vulnerabilities
• Bugs that break functionality
• Breaking changes without migration path
• Missing tests for critical features

**Blocking:** This exposes user emails without authentication. 
We need to add auth middleware here before merging.

2. Important Suggestions (Should Fix)

Improvements that significantly impact quality:

• Performance issues
• Poor code structure
• Confusing naming
• Missing error handling

**Suggestion:** This N+1 query will slow down with many users. 
Consider using a join or eager loading to fetch in one query?

3. Nits (Nice to Have)

Minor improvements that don't block merging:

• Style preferences (if no auto-formatter)
• Minor naming improvements
• Optional refactorings

**Nit:** We typically use camelCase for function names here, 
but not a blocker if you prefer snake_case.

4. Questions

Ask for clarification:

**Question:** Why did you choose to use setTimeout here instead 
of async/await? Just curious about the approach!

5. Praise

Acknowledge good work:

**Nice!** This is a really elegant solution to the race condition. 
Love the use of debouncing here. 👍

Code Review Checklist

Use this checklist for consistent, thorough reviews:

Common Review Pitfalls to Avoid

1. Rubber Stamping

Approving without actually reviewing:

2. Perfectionism

Demanding perfect code before merging:

3. Style Bikeshedding

Arguing about trivial style preferences:

4. Design Reviews During Code Review

Questioning fundamental approach after code is written:

5. Being a Gatekeeper

Using reviews to assert dominance:

Common Review Scenarios

Scenario 1: Everything Looks Good

Great work! The code is clean, well-tested, and solves the issue 
effectively. I particularly like how you handled the edge case in 
line 45. Approving! 👍

Scenario 2: Minor Issues Only

Overall this looks good! I have a few minor suggestions:

1. Consider renaming `data` to `userProfiles` for clarity (line 23)
2. This error message could be more descriptive (line 67)

None of these block merging though - feel free to merge as-is or 
address them, your choice! Approving.

Scenario 3: Needs Changes

Thanks for the PR! I've found a few issues that need addressing:

**Blocking:**
- Lines 45-50: This doesn't handle the null case, which will crash 
  when users have no profile

**Suggestions:**
- Line 78: Consider caching this API call - it's called on every render
- Lines 100-120: This logic could be simplified

Let me know if you have questions on any of this! Happy to discuss.

Scenario 4: Major Architectural Concerns

I appreciate the work here, but I have concerns about the overall 
approach. This tightly couples the auth logic to the component, which 
will make testing difficult and doesn't match our architecture pattern.

Can we hop on a quick call to discuss? I want to make sure we align on 
the approach before you put more time into this. My fault for not 
catching this earlier in the design phase.

Practice Review Commands

Try these commands for reviewing PRs locally:

Code Review Etiquette

For Reviewers:

• Review promptly: Don't leave PRs hanging for days
• Be kind: Remember there's a person on the other end
• Assume competence: The author probably had good reasons
• Ask, don't command: "What do you think?" not "You must"
• Explain why: Don't just say what to change
• Praise good work: Acknowledge what's done well
• Know when to discuss live: If review has 20+ comments, hop on a call

For Authors Receiving Reviews:

• Don't take it personally: It's about the code, not you
• Appreciate the time: Reviews take effort
• Ask questions: If you don't understand feedback
• Push back respectfully: If you disagree, explain why
• Learn from feedback: Reviews help you grow
• Respond to each comment: Even if just "Done!"

Key Takeaways

What's Next?

Excellent work! You now understand how to review pull requests effectively, give constructive feedback, and be a valuable collaborator. Code review skills make you a better developer and teammate!

In the next lesson, we'll tackle Handling Merge Conflicts. You'll learn what causes conflicts, how to resolve them step-by-step, understand conflict markers, use merge tools, and prevent conflicts from happening in the first place. Merge conflicts sound scary, but they're actually quite manageable once you understand them!