Code Reviews¶

Progress: Module 4 of 4 - Lesson 2 of 3

Master the art of reviewing code effectively, providing constructive feedback, and maintaining code quality standards across your team.

Prerequisites¶

Before diving into code reviews, ensure you have:

• Completed GitHub Workflow lesson
• Basic understanding of your team's codebase
• Familiarity with your team's coding standards
• Access to your team's GitHub repository
• Understanding of testing practices

Core Concepts¶

Purpose of Code Reviews¶

Code reviews serve multiple purposes:

1. Quality Assurance
2. Catch bugs before production
3. Ensure code meets standards

4. Prevent technical debt

5. Knowledge Sharing

6. Team learns from each other
7. Spreads expertise across codebase

8. Reduces single points of knowledge

9. Risk Mitigation

10. Multiple eyes on critical code
11. Encourages security awareness

12. Validates architectural decisions

13. Team Culture

14. Builds psychological safety
15. Encourages best practices
16. Mentors junior developers

Team Collaboration Best Practices¶

As a Reviewer¶

1. Be Thorough But Time-Bound¶

Good Review Cycle: - Reviews should take 15-30 minutes - Review within 24 hours of request - If it's taking longer, ask for clarification or a smaller PR

What to Check:

Functionality
├── Does the code do what the PR claims?
├── Are edge cases handled?
├── Is error handling appropriate?
└── Are there any performance issues?

Code Quality
├── Is the code readable?
├── Are variables/functions well-named?
├── Is there unnecessary duplication?
└── Does it follow team patterns?

Testing
├── Are tests comprehensive?
├── Do they actually test the new code?
├── Are edge cases covered?
└── Is test coverage maintained?

Security
├── No hardcoded secrets/credentials?
├── Input validation present?
├── No SQL injection vulnerabilities?
└── Is authentication/authorization correct?

Documentation
├── Comments explain the "why", not "what"
├── Public APIs documented?
├── Complex algorithms explained?
└── Breaking changes documented?

2. Give Constructive Feedback¶

Good Feedback:

"The parameter naming here is unclear. Consider renaming
'val' to 'userScore' to make the calculation more obvious."

Poor Feedback:

"Bad naming"

Structure: 1. Acknowledge what's good 2. Explain the issue 3. Suggest improvement 4. Explain why it matters

Example:

"I like how you handled the error cases here.
For consistency with other endpoints, consider
using the Response.error() helper instead of
throwing directly. This keeps our error format consistent."

3. Distinguish Between Issues and Preferences¶

Blocking Issues (must fix): - Security vulnerabilities - Production bugs - Breaks tests - Violates architecture patterns

Nice-to-Have (suggest): - Style preferences - Micro-optimizations - Documentation improvements

Use labels:

"[BLOCKING] This breaks the existing test suite"
"[SUGGESTION] Consider extracting this to a helper function"
"[DISCUSSION] I'm not sure about this approach. What do you think?"

4. Approve Only When Ready¶

Check: - [ ] Functionality is correct - [ ] Tests pass - [ ] Code quality acceptable - [ ] No security issues - [ ] Follows team standards - [ ] Documentation adequate

Approval comment:

"Looks good! This is ready to merge once tests pass."

As a Code Author¶

1. Make Code Easy to Review¶

Optimal PR Size: - 200-400 lines of actual code change - Takes 15-30 minutes to review - Focused on one feature/fix

Bad PR: - 2000+ lines of changes - Mixes refactoring with features - Spans multiple concerns

Good PR: - Feature addition with tests - Bug fix with reproduction test - Clear description explaining the why

2. Provide Context in PR Description¶

## What This PR Does
Brief description of the change.

## Why We Need This
Context and motivation. Link to related issues.

## How to Test
Steps to verify the change works.

## Screenshots/Demos
If UI changes, include screenshots.

## Deployment Notes
Any special deployment considerations?
Breaking changes?
Database migrations?

3. Respond to Feedback Promptly¶

Good Response:

"Good catch! I've updated the error handling to match
the pattern in utils/errorHandler.ts. Check the latest commit."

Poor Response:

"ok"

When You Disagree:

"I appreciate the suggestion, but I think the current approach
is better because [explain reasoning]. What do you think?"

4. Keep PRs Current¶

# Sync with main regularly
git fetch origin
git rebase origin/main
git push origin --force-with-lease

# Don't let PRs sit for more than a few days

Code Review Checklist¶

Functionality Review¶

• Does the code implement the intended feature/fix?
• Are edge cases handled correctly?
• Does it work with different input values?
• Are boundary conditions checked?
• Is error handling appropriate and user-friendly?
• Are there any performance issues?
• Does it maintain backward compatibility (if needed)?

Code Quality Review¶

• Is the code readable and well-structured?
• Are variable and function names clear and descriptive?
• Are there any obvious bugs or logic errors?
• Is there unnecessary code or duplication?
• Does it follow team coding conventions?
• Are there any code smells or anti-patterns?
• Could complex code be simplified?

Testing Review¶

• Are tests included for new functionality?
• Do tests actually verify the new code?
• Are edge cases and error scenarios tested?
• Are there any obviously failing tests?
• Is test coverage maintained or improved?
• Are integration tests sufficient?
• Do tests follow team patterns?

Security Review¶

• Are no secrets/credentials hardcoded?
• Is all user input validated?
• Are SQL injection vulnerabilities prevented?
• Is authentication/authorization correctly implemented?
• Are sensitive operations properly logged?
• Are external API calls secure?
• Are error messages non-revealing?

Documentation Review¶

• Are comments explaining "why" not just "what"?
• Are public APIs documented?
• Are complex algorithms explained?
• Are breaking changes documented?
• Is the PR description clear and helpful?
• Are commit messages descriptive?
• Are architecture decisions documented?

Common Review Scenarios¶

Scenario 1: Performance Concern¶

Observation:

const users = await User.find({}); // Gets ALL users
const active = users.filter(u => u.active === true);

Feedback:

"This loads all users into memory then filters.
Consider filtering at the database level:

const active = await User.find({ active: true });

This is more efficient, especially with large datasets."

Scenario 2: Security Issue¶

// User input directly in SQL
const user = await db.query(`SELECT * FROM users WHERE id = ${userId}`);

Feedback:

"[BLOCKING] This is a SQL injection vulnerability.
Use parameterized queries:

const user = await db.query('SELECT * FROM users WHERE id = ?', [userId]);

Scenario 3: Inconsistent Patterns¶

// Using different error handling pattern than rest of codebase
if (!user) {
  res.status(404).json({ error: 'Not found' });
}

Feedback:

"[SUGGESTION] For consistency with other endpoints,
consider using the errorHandler middleware:

if (!user) {
  throw new NotFoundError('User not found');
}

This keeps our error handling patterns consistent across the app."

Scenario 4: Missing Test Coverage¶

function calculateDiscount(price, isVIP) {
  if (isVIP) return price * 0.9;
  return price * 0.95;
}

Feedback:

"Nice addition! I'd like to see tests for this logic.
Could you add a test for:
- VIP users get 10% discount
- Regular users get 5% discount
- Edge case with zero price
- Negative prices (invalid input)"

Review Tone and Culture¶

Use Positive Language¶

Instead of: "This is wrong" Try: "I think this could be improved"

Instead of: "You forgot tests" Try: "Let's add tests for this scenario"

Instead of: "This is inefficient" Try: "I wonder if we could optimize this further"

Celebrate Good Work¶

"This is really clean implementation!
I like how you handled the async flow."

Learn from Each Other¶

"I learned something new from this!
Could you explain your approach to [concept]?"

AI Prompts for Code Reviews¶

Reviewing Pull Requests Effectively¶

I need to review a pull request but want to be thorough.

PR details:
- Title: [PR title]
- Changes: [describe type of changes]
- Files modified: [number] files

File changes summary:
[paste the diff summary or key files changed]

Help me create a review that checks:
1. Code quality and style
2. Security concerns
3. Performance implications
4. Test coverage
5. Documentation needs

Provide a checklist specific to this type of change.

I'm reviewing a PR that touches authentication/authorization.

Changes include:
[describe changes]

Files modified:
[list security-related files]

Help me review for:
- Common auth vulnerabilities
- Token/session handling issues
- Input validation problems
- Authorization bypass risks
- Secrets management

What specific security checks should I perform?

This PR modifies database queries and API endpoints.

Code snippet:
[paste relevant code]

Database operations:
[describe queries/operations]

Help me identify:
1. N+1 query problems
2. Missing indexes
3. Inefficient loops
4. Memory leaks
5. API response time impacts

Suggest optimizations with examples.

Giving Constructive Feedback¶

I found issues in a PR but want to give constructive feedback.

Issues I found:
1. [describe issue]
2. [describe issue]

Help me write comments that:
- Are specific and actionable
- Explain WHY something is an issue
- Suggest solutions, not just problems
- Are encouraging, not discouraging

Draft example comments for each issue.

A teammate implemented [feature/fix] but I think there's a better approach.

Their approach:
[describe current implementation]

My suggested approach:
[describe alternative]

Help me write a comment that:
- Acknowledges their work
- Explains trade-offs clearly
- Suggests without demanding
- Opens discussion, not closes it

I found these issues in a PR:

1. [issue description]
2. [issue description]
3. [issue description]

Help me categorize each as:
- [BLOCKING] - must fix before merge
- [SUGGESTION] - nice to have, not required
- [DISCUSSION] - open for debate

Explain your reasoning for each categorization.

Responding to Feedback as Author¶

I received these review comments on my PR:

1. [reviewer comment]
2. [reviewer comment]

Help me craft responses that:
- Show I understand the concern
- Explain my changes or reasoning
- Are collaborative, not defensive
- Move the PR forward

Draft example responses for each.

A reviewer suggested:
[reviewer's suggestion]

I disagree because:
[your reasoning]

Help me respond in a way that:
- Respects their expertise
- Clearly explains my perspective
- Offers to discuss further
- Finds common ground

How can I disagree without being dismissive?

Learning from Code Reviews¶

I'm reviewing code using patterns I'm not familiar with:

[paste code snippet]

The pattern/library used: [name]

Help me understand:
1. What this code does
2. Why this pattern might be chosen
3. What to look for when reviewing it
4. Common mistakes with this pattern
5. How to verify it's implemented correctly

FAQ¶

Q: How fast should I review a PR? A: Within 24 hours is a good target. If code is blocked, prioritize.

Q: Should I approve if I have minor suggestions? A: Yes, if they're not blocking. Use "Request Changes" only for important issues.

Q: What if I disagree with the approach? A: Discuss it! Ask questions about the reasoning. Suggest alternatives.

Q: How detailed should my review comments be? A: Detailed enough that the author understands your concern and how to fix it.

Q: Should I review code from senior developers differently? A: Focus on the code, not the person. Review with the same standards.

Q: What if I'm not an expert in that part of the codebase? A: Review what you can, flag areas for domain experts, don't block without reason.

What's Next¶

Now that you understand code reviews, move on to:

Deployment Process →

Learn about safe deployment practices and rollback procedures that keep your production systems reliable.

Help & Resources¶

Improving Your Reviews? - Ask senior developers to review your reviews - Study excellent code in your codebase - Practice explaining your reasoning clearly

Building Team Culture? - Emphasize learning, not criticism - Celebrate good practices you see - Share knowledge in reviews

Want to Learn More? - Study famous codebases (Linux, React, etc.) - Learn pair programming techniques - Explore automated code quality tools

Next Lesson: Deployment Process →