Farhan Sadeek

Student

The Art of Code Reviews

Code reviews are one of the most powerful tools for improving code quality, sharing knowledge, and building better software teams. When done well, they catch bugs, improve maintainability, and foster a culture of learning. Let's explore how to make code reviews effective and constructive.

Why Code Reviews Matter

Quality Assurance

  • Bug detection - Fresh eyes catch issues the author missed
  • Edge case identification - Reviewers think of scenarios the author didn't consider
  • Performance optimization - Different perspectives on efficiency
  • Security vulnerabilities - Additional security scrutiny

Knowledge Sharing

  • Domain expertise - Share context and business logic
  • Technical patterns - Spread best practices across the team
  • Language features - Learn new approaches and techniques
  • Architecture understanding - Align on system design decisions

Team Building

  • Collective ownership - Everyone shares responsibility for the codebase
  • Mentorship opportunities - Senior developers guide junior team members
  • Communication skills - Practice giving and receiving feedback
  • Trust building - Develop confidence in each other's work

Preparing for Code Reviews

For Authors

Self-Review First

# Before submitting, review your own changes
git diff main...feature-branch

# Check for common issues
- Commented-out code
- Debug statements
- Formatting inconsistencies
- Missing tests

Write Clear Descriptions

## What this PR does

Implements user authentication using JWT tokens

## Why these changes are needed

- Users need to log in to access protected features
- Replaces the old session-based authentication
- Improves security with token expiration

## How to test

1. Run `npm test` for unit tests
2. Start the dev server and try logging in
3. Check that tokens expire after 24 hours

## Screenshots/Examples

[Include relevant screenshots or code examples]

Keep PRs Focused

// ❌ Bad: Multiple unrelated changes
class UserService {
  // Authentication logic
  async login(email: string, password: string) { ... }

  // Profile management (should be separate PR)
  async updateProfile(userId: string, data: ProfileData) { ... }

  // Email notifications (should be separate PR)
  async sendWelcomeEmail(user: User) { ... }
}

// ✅ Good: Single responsibility
class AuthenticationService {
  async login(email: string, password: string) { ... }
  async logout(token: string) { ... }
  async refreshToken(token: string) { ... }
}

For Reviewers

Understand the Context

// Before reviewing, understand:
// 1. What problem is being solved?
// 2. How does this fit into the larger system?
// 3. Are there any constraints or requirements?

// Read the PR description and linked issues
// Check the diff to understand the scope
// Look at the test cases to understand expected behavior

Effective Review Techniques

Focus on the Right Things

High Priority

  • Logic errors and bugs
  • Security vulnerabilities
  • Performance issues
  • API design and interfaces
  • Test coverage and quality
// ❌ Security issue to flag
function getUser(userId: string) {
  const query = `SELECT * FROM users WHERE id = ${userId}`
  return database.query(query) // SQL injection vulnerability
}

// ✅ Secure alternative
function getUser(userId: string) {
  const query = 'SELECT * FROM users WHERE id = ?'
  return database.query(query, [userId])
}

Medium Priority

  • Code organization and structure
  • Naming conventions
  • Documentation and comments
  • Error handling

Low Priority

  • Code style (should be automated)
  • Personal preferences
  • Nitpicks that don't affect functionality

Constructive Feedback Patterns

Be Specific and Actionable

# ❌ Vague feedback

"This function is confusing"

# ✅ Specific and actionable

"The function name `processData` doesn't clearly indicate what kind of processing happens. Consider renaming to `validateAndFormatUserInput` to better describe its purpose."

Explain the Why

# ❌ Just pointing out issues

"Don't use `any` here"

# ✅ Explaining the reasoning

"Using `any` here bypasses TypeScript's type checking, which could lead to runtime errors. Consider using a more specific type like `User | null` or creating an interface for the expected shape."

Suggest Solutions

// ❌ Just criticizing
// "This will cause performance issues"

// ✅ Providing alternatives
// "This loop will run on every render. Consider memoizing with useMemo:"

// Current code
function ExpensiveComponent({ items }) {
  const processedItems = items.map(item => expensiveTransform(item))
  return <div>{processedItems}</div>
}

// Suggested improvement
function ExpensiveComponent({ items }) {
  const processedItems = useMemo(
    () => items.map(item => expensiveTransform(item)),
    [items]
  )
  return <div>{processedItems}</div>
}

Question vs. Command

# ❌ Commands (can feel aggressive)

"Change this to use async/await"
"Remove this console.log"

# ✅ Questions (encourages discussion)

"What do you think about using async/await here for better readability?"
"Should we remove this console.log before merging?"

# ✅ Suggestions (collaborative tone)

"Consider using async/await here for better error handling"
"Suggestion: We could extract this logic into a helper function"

Review Checklist

Functionality

  • [ ] Does the code do what it's supposed to do?
  • [ ] Are edge cases handled appropriately?
  • [ ] Is error handling comprehensive?
  • [ ] Are the tests sufficient and passing?

Code Quality

  • [ ] Is the code readable and well-organized?
  • [ ] Are functions and variables named clearly?
  • [ ] Is the code DRY (Don't Repeat Yourself)?
  • [ ] Are there any code smells or anti-patterns?

Performance

  • [ ] Are there any obvious performance issues?
  • [ ] Is the solution appropriately efficient?
  • [ ] Are expensive operations optimized or cached?

Security

  • [ ] Are user inputs validated and sanitized?
  • [ ] Are there any potential security vulnerabilities?
  • [ ] Is sensitive data handled appropriately?

Maintainability

  • [ ] Will this code be easy to modify in the future?
  • [ ] Is it documented where necessary?
  • [ ] Does it follow the project's conventions?

Handling Disagreements

When You Disagree as a Reviewer

# ✅ Express disagreement constructively

"I understand the approach, but I'm concerned about maintainability. What if we tried X instead? Here's why I think it might be better..."

# ✅ Ask for clarification

"Help me understand why you chose this approach over Y. I might be missing some context."

# ✅ Suggest alternatives

"Have you considered using the existing utility function for this? It might reduce duplication."

When Your Code is Questioned

# ✅ Be open to feedback

"Good point! I hadn't considered that edge case. Let me add some validation."

# ✅ Explain your reasoning

"I chose this approach because of performance requirements, but I'm open to alternatives if you see issues with it."

# ✅ Ask for help

"I'm not sure about the best way to handle this. Do you have suggestions?"

Advanced Review Techniques

Pair Review Sessions

For complex changes, consider real-time review:

# Schedule a shared screen session
# Walk through the code together
# Discuss trade-offs and alternatives
# Make decisions collaboratively

Incremental Reviews

For large features:

## Phase 1: Architecture Review

- Review the overall approach
- Validate design decisions
- Ensure alignment with requirements

## Phase 2: Implementation Review

- Review specific implementation details
- Check for bugs and edge cases
- Validate test coverage

## Phase 3: Integration Review

- Review how components work together
- Check for integration issues
- Validate end-to-end functionality

Context-Aware Reviews

Tailor your review style to the situation:

// Junior developer's code: Focus on learning
// - Explain best practices
// - Suggest learning resources
// - Be extra patient and encouraging

// Senior developer's code: Focus on architecture
// - Challenge design decisions
// - Discuss trade-offs
// - Ensure consistency with team standards

// Urgent bug fix: Focus on correctness
// - Verify the fix addresses the issue
// - Check for regressions
// - Ensure minimal scope

Common Pitfalls to Avoid

Review Fatigue

❌ Don't:

- Review too many PRs at once
- Rush through large changes
- Review when tired or distracted

✅ Do:

- Take breaks between reviews
- Set aside dedicated time for reviews
- Ask for help with large or complex PRs

Bikeshedding

❌ Avoid spending excessive time on:

- Minor style preferences
- Subjective naming debates
- Personal coding preferences

✅ Focus on:

- Functional correctness
- Security and performance
- Maintainability and clarity

Personal Attacks

❌ Never:

- Attack the person ("You always...")
- Make assumptions about intent
- Use absolute language ("This is terrible")

✅ Always:

- Focus on the code, not the person
- Assume positive intent
- Use collaborative language ("We could...")

Tools and Automation

Automated Checks

# GitHub Actions example
name: Code Review Automation
on: pull_request

jobs:
  lint:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v2
      - name: Run linter
        run: npm run lint

  test:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v2
      - name: Run tests
        run: npm test

  security:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v2
      - name: Security audit
        run: npm audit

Review Assignment

# CODEOWNERS file
# Global owners
* @senior-dev @team-lead

# Frontend specific
/src/components/ @frontend-team @ui-specialist

# Backend specific
/src/api/ @backend-team @api-specialist

# Database migrations
/migrations/ @database-admin @backend-lead

Measuring Review Effectiveness

Metrics to Track

  • Review turnaround time - How quickly are reviews completed?
  • Defect detection rate - How many bugs are caught in review?
  • Review coverage - What percentage of code is reviewed?
  • Team satisfaction - How do developers feel about the review process?

Continuous Improvement

## Regular Retrospectives

- What's working well in our review process?
- What could be improved?
- Are we catching the right kinds of issues?
- How can we make reviews more effective?

## Team Guidelines

- Document your team's review standards
- Regularly update based on learnings
- Share knowledge about effective techniques
- Celebrate good review practices

Conclusion

Effective code reviews are an art that combines technical skill, communication, and empathy. They're not just about finding bugs—they're about building better software and stronger teams.

Key principles to remember:

  1. Be constructive - Focus on helping, not criticizing
  2. Be specific - Give actionable feedback
  3. Be collaborative - Work together toward better solutions
  4. Be respectful - Treat others as you'd like to be treated
  5. Be learning-oriented - See reviews as opportunities to grow

When done well, code reviews become a cornerstone of engineering culture, fostering continuous learning, maintaining high standards, and building trust within the team.

Remember: every review is an opportunity to make the codebase better and help your teammates grow. Approach each one with that mindset, and you'll build software—and teams—that truly excel.