Pull Requests and Code Reviews

Module 2: Version Control & Containerization

Introduction to Pull Requests

In our previous lecture, we introduced GitHub as a platform and briefly touched on pull requests as a key collaboration feature. Today, we'll dive deep into pull requests and code reviews, which form the backbone of collaborative development on GitHub and similar platforms.

Pull requests (often abbreviated as PRs) are more than just a mechanism for merging code—they're a communication tool, a documentation system, and a quality control checkpoint all in one. Think of them as a formal proposal to introduce changes, accompanied by a structured discussion forum.

flowchart TD A[Developer Creates Branch] --> B[Implements Changes] B --> C[Submits Pull Request] C --> D[Team Reviews Code] D --> E{Approved?} E -->|No| F[Address Feedback] F --> D E -->|Yes| G[Merge Changes] G --> H[Delete Branch] style C fill:#e1f5fe,stroke:#0288d1,stroke-width:2px style D fill:#e8f5e9,stroke:#43a047,stroke-width:2px style E fill:#fff3e0,stroke:#ff9800 style G fill:#f3e5f5,stroke:#8e24aa

By the end of this lecture, you'll understand how to create effective pull requests, review code thoughtfully, and use these tools to maintain high-quality code while collaborating efficiently with others.

Pull Request Anatomy

Let's examine the components of a pull request in detail to understand what makes them such powerful collaboration tools.

The Basic Structure

A pull request consists of several key elements:

Together, these elements create a complete picture of what's being changed, why, and what the community thinks about it.

PR Lifecycle

A typical pull request goes through several stages:

stateDiagram-v2 [*] --> Draft: Create PR as draft [*] --> Open: Create PR directly Draft --> Open: Mark as ready for review Open --> In_Review: Reviewer starts review In_Review --> Changes_Requested: Request changes Changes_Requested --> Open: Author makes changes In_Review --> Approved: Approve PR Approved --> Merged: Merge PR Open --> Closed: Close without merging state Open { [*] --> Awaiting_Review Awaiting_Review --> Under_Review }
  1. Creation: The PR is created, either as a draft or ready for review
  2. Review: Reviewers examine the code and provide feedback
  3. Revision: The author addresses feedback with additional commits
  4. Approval: Reviewers approve the changes
  5. Merge: The changes are incorporated into the target branch
  6. Cleanup: The source branch is typically deleted after merging

Some PRs might go through several rounds of review and revision before being approved and merged.

Types of Pull Requests

Pull requests can serve different purposes:

The type of PR often influences how it's reviewed and how quickly it's processed.

Pull Request Templates

PR templates help ensure that authors provide all necessary information. Here's an example of a comprehensive template:

## Description
Please provide a brief description of the changes in this pull request.

## Related Issue
Fixes #(issue number)

## Type of Change
- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
- [ ] This change requires a documentation update

## How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration.

## Checklist:
- [ ] My code follows the style guidelines of this project
- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation
- [ ] My changes generate no new warnings
- [ ] I have added tests that prove my fix is effective or that my feature works
- [ ] New and existing unit tests pass locally with my changes
- [ ] Any dependent changes have been merged and published in downstream modules

To add a template to your repository, create a file at .github/PULL_REQUEST_TEMPLATE.md.

Draft Pull Requests

Draft PRs allow you to create a pull request that is explicitly not ready for review yet:

To create a draft PR, select "Create draft pull request" when creating the PR, or click "Convert to draft" on an existing PR.

Creating Effective Pull Requests

The way you structure and present your pull requests can significantly impact the review process and the likelihood of your changes being accepted. Let's explore best practices for creating effective PRs.

Preparation Before Creating a PR

Before submitting your pull request, take these preparatory steps:

Writing Clear PR Descriptions

A good PR description should:

Example of a well-written PR description:

## User Authentication Implementation

This PR implements a complete user authentication system using JWT tokens. It allows users to sign up, log in, and access protected routes with valid tokens.

### Changes Made
- Add User model with secure password hashing
- Create authentication controller with signup and login endpoints
- Implement JWT token generation and validation
- Add middleware for protecting routes
- Set up automated tests for authentication flows

### Why These Changes Are Needed
Issue #42 identified the need for a secure authentication system before we release the beta version.

### Implementation Details
- Uses bcrypt for password hashing with a cost factor of 12
- JWT tokens expire after 24 hours
- Includes email validation during signup
- Uses Redis for token blacklisting on logout

### Testing Done
- Added unit tests for all authentication functions
- Manual testing with Postman for all happy paths and common error scenarios
- Load tested with 1000 concurrent authentication requests

### Potential Concerns
- The Redis integration might need configuration updates in production
- We may want to adjust token expiration times based on user feedback

### Screenshots
![Login Flow](https://link-to-screenshot.png)

Keeping PRs Focused and Manageable

Large, unfocused PRs are difficult to review and more likely to introduce bugs. To keep your PRs manageable:

Example of breaking down a large feature:

flowchart TD A[User Management Feature] --> B[PR 1: Database Schema] A --> C[PR 2: Backend API] A --> D[PR 3: Authentication] A --> E[PR 4: Frontend UI] A --> F[PR 5: Email Notifications] B -.-> C C -.-> D D -.-> E E -.-> F style A fill:#f9f9f9,stroke:#333,stroke-width:2px style B fill:#e1f5fe,stroke:#0288d1 style C fill:#e1f5fe,stroke:#0288d1 style D fill:#e1f5fe,stroke:#0288d1 style E fill:#e1f5fe,stroke:#0288d1 style F fill:#e1f5fe,stroke:#0288d1

Making PRs Easier to Review

Help reviewers by making your PR easy to understand and evaluate:

Consider adding a special "Notes for Reviewers" section in complex PRs to guide the review process.

Addressing Feedback

When you receive feedback on your PR, handle it professionally:

Example response to feedback:

Thanks for the feedback! I've made the following changes:

1. Fixed the validation logic as suggested
2. Added tests for the edge cases you mentioned
3. Improved the error messages to be more user-friendly

Regarding the suggested refactoring of the authentication flow, I think that's a great idea but it might be better as a separate PR since it would touch multiple systems. What do you think?

Code Review Fundamentals

Code review is a systematic examination of code to find and fix mistakes, improve code quality, and ensure adherence to standards. Let's explore the foundations of effective code reviews.

The Purpose of Code Reviews

Code reviews serve multiple important purposes:

Effective code reviews balance these different purposes to maximize the value derived from the process.

What to Look for in a Code Review

When reviewing code, consider these aspects:

It's helpful to have a checklist or guide to ensure you cover all relevant aspects during a review.

Code Review Checklist

Here's a sample checklist to guide your code reviews:

## Code Review Checklist

### Functionality
- [ ] The code works as expected
- [ ] Edge cases are handled appropriately
- [ ] Error cases are handled gracefully
- [ ] The code handles concurrent operations safely if applicable

### Code Quality
- [ ] The code follows the project's style guide
- [ ] Variable, function, and class names are clear and meaningful
- [ ] There is no unnecessary duplication
- [ ] The code is as simple as possible but no simpler
- [ ] There are no commented-out code blocks
- [ ] There are no debug statements (e.g., console.log, print)

### Architecture and Design
- [ ] The code follows SOLID principles
- [ ] The code uses appropriate design patterns
- [ ] New components fit well with the existing architecture
- [ ] Dependencies are appropriate and minimized

### Performance
- [ ] The code doesn't perform unnecessary operations
- [ ] Expensive operations are optimized
- [ ] Database queries are efficient
- [ ] Resources are properly released

### Security
- [ ] User input is validated and sanitized
- [ ] Authentication and authorization are properly implemented
- [ ] Sensitive data is handled securely
- [ ] The code doesn't introduce new vulnerabilities

### Testing
- [ ] The code includes appropriate tests
- [ ] Tests cover normal cases, edge cases, and error cases
- [ ] Tests are clear and maintainable
- [ ] All tests pass

### Documentation
- [ ] Code includes necessary comments
- [ ] API documentation is updated
- [ ] README and other documentation is updated if necessary

Adapt this checklist to your project's specific needs and priorities.

Different Types of Code Reviews

Code reviews can take different forms, each with its own strengths:

Many teams use a combination of these approaches depending on the situation.

Code Review Roles

In a typical code review process, there are several roles:

Often, the same person may play multiple roles on different PRs, rotating responsibilities within the team.

Performing Effective Code Reviews

The way you conduct a code review significantly impacts its effectiveness and the team's experience. Let's explore best practices for reviewing code.

Code Review Mindset

Approach code reviews with the right mindset:

This mindset helps create a positive and productive review environment.

The Review Process

Follow a structured process when reviewing code:

  1. Context Understanding: Read the PR description and linked issues
  2. Big Picture Review: Look at the overall changes before diving into details
  3. Detailed Examination: Go through the code line by line
  4. Testing Verification: Check that tests are adequate and pass
  5. Documentation Check: Ensure documentation is updated appropriately
  6. Feedback Compilation: Organize your thoughts into clear feedback
  7. Follow-up: Be available to discuss and clarify your feedback

This systematic approach helps ensure a thorough and effective review.

Providing Constructive Feedback

The way you phrase your feedback dramatically affects how it's received and acted upon:

Constructive Feedback Examples

Instead of Try
"This code is a mess." "This might be easier to understand if we restructured it like this..."
"Why would you do it this way?" "I'm curious about the reasoning behind this approach."
"You forgot to handle this edge case." "It looks like we need to handle the case when the input is empty."
"This is wrong." "I think there might be an issue here because..."
"Never use global variables." "Using a local variable here would prevent potential side effects."

Feedback Guidelines

Example of well-structured feedback:

I've reviewed your authentication implementation and have a few thoughts:

### Strengths
- The token generation logic is clean and well-tested
- Good job separating the authentication logic into its own middleware
- I like the clear error messages for different authentication failures

### Suggestions
- In the login controller, we might want to rate-limit login attempts to prevent brute force attacks
- The password validation could be strengthened with a minimum complexity requirement
- Consider adding a refresh token mechanism for better security

### Questions
- What was the reasoning behind the 24-hour token expiration? Would a shorter time improve security without affecting UX too much?
- Have we considered how this would scale across multiple servers?

Let me know what you think!

Using GitHub's Review Features

GitHub provides several tools to make the review process more efficient:

Line Comments

Click on the "+" icon that appears when hovering over a line to add a comment specific to that line.

Suggestion Feature

Use the suggestion feature to propose specific code changes:

  1. Click the "+" icon next to a line
  2. Click the "suggestion" button (looks like a pencil)
  3. Edit the code as needed
  4. Add any explanatory comments
  5. Submit the suggestion

The author can then directly accept your suggestion with a single click.

File View Options

Use these options to make reviewing easier:

Review Summary

When you're done reviewing, submit an overall review:

  1. Click "Review changes" at the top right of the PR
  2. Add summary comments
  3. Choose a review type:
    • Comment: General feedback without explicit approval
    • Approve: Approve the changes and allow merging
    • Request changes: Indicate that changes are needed before merging
  4. Submit the review

Balancing Thoroughness and Speed

One of the challenges in code reviews is finding the right balance between thoroughness and timely feedback:

Aim to provide initial feedback within one business day to keep development flowing.

Handling Code Review Challenges

Code reviews can sometimes be challenging, both technically and interpersonally. Let's discuss how to handle common issues.

Dealing with Large PRs

Very large pull requests are difficult to review effectively. When faced with a massive PR:

For the future, work with the team to establish guidelines for PR size and scope.

Handling Disagreements

Disagreements during code reviews are normal and can be productive if handled well:

Example approach to a disagreement:

I see your point about using a factory pattern here. My concern is that it adds complexity for our specific use case. Perhaps we could discuss this in person? I'd like to understand your perspective better and see if we can find an approach that balances simplicity and flexibility.

Managing Review Fatigue

Review fatigue can reduce the effectiveness of code reviews. To combat it:

Remember that a tired reviewer might miss important issues, so managing fatigue is crucial for quality.

Mentoring Through Code Reviews

Code reviews provide excellent opportunities for mentorship:

Example of mentorship-oriented feedback:

I notice we're using a lot of nested if statements here. This can make the code harder to follow as complexity grows. Have you come across the "early return" pattern? It can help flatten the code and make the logic clearer. Here's an example:

```javascript
// Instead of:
function processUser(user) {
  if (user) {
    if (user.isActive) {
      if (user.hasPermission) {
        // Do something
        return true;
      } else {
        return false;
      }
    } else {
      return false;
    }
  } else {
    return false;
  }
}

// Consider:
function processUser(user) {
  if (!user) return false;
  if (!user.isActive) return false;
  if (!user.hasPermission) return false;
  
  // Do something
  return true;
}
```

This article explains the pattern in more detail: [link]

What do you think about this approach?

Security and Compliance Considerations

Some projects require special attention to security and compliance issues:

For regulated industries or sensitive applications, consider implementing specialized review processes.

Automating Code Reviews

Automation can significantly enhance the code review process by catching issues early and freeing reviewers to focus on higher-level concerns.

Static Analysis Tools

Static analysis tools analyze code without executing it, finding potential issues:

These tools can be run locally during development and as part of automated CI checks.

Automated Tests in Review

Automated tests provide confidence that code changes work as expected:

Automated tests should run as part of the CI process for every pull request.

Setting Up GitHub Checks

GitHub Checks provide feedback directly in the PR interface:

GitHub Actions Example

name: Code Quality Checks

on:
  pull_request:
    branches: [ main, develop ]

jobs:
  lint:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v3
      - name: Set up Node.js
        uses: actions/setup-node@v3
        with:
          node-version: '16'
      - name: Install dependencies
        run: npm ci
      - name: Run linting
        run: npm run lint
        
  test:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v3
      - name: Set up Node.js
        uses: actions/setup-node@v3
        with:
          node-version: '16'
      - name: Install dependencies
        run: npm ci
      - name: Run tests
        run: npm test
        
  security:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v3
      - name: Run security scan
        uses: snyk/actions/node@master
        env:
          SNYK_TOKEN: ${{ secrets.SNYK_TOKEN }}

Required Checks

You can configure GitHub to require certain checks to pass before a PR can be merged:

  1. Go to the repository's Settings tab
  2. Click on Branches in the sidebar
  3. Under "Branch protection rules," select the branch (e.g., main)
  4. Check "Require status checks to pass before merging"
  5. Select the checks that must pass

This ensures that PRs can only be merged when they meet your quality standards.

Code Review Bots

Specialized bots can further automate the review process:

Example Danger.js file to enforce PR best practices:

import {danger, fail, warn, message} from 'danger';

// Require PR description
if (danger.github.pr.body.length < 10) {
  fail('Please provide a meaningful description for this PR.');
}

// Check for test files
const changedFiles = [...danger.git.modified_files, ...danger.git.created_files];
const hasTestChanges = changedFiles.some(file => file.includes('test') || file.includes('spec'));
const hasSourceChanges = changedFiles.some(file => 
  file.includes('src/') && 
  !file.includes('test') && 
  !file.includes('spec')
);

if (hasSourceChanges && !hasTestChanges) {
  warn('This PR contains source code changes, but no tests. Consider adding tests.');
}

// Check PR size
const bigPRThreshold = 500;
const linesChanged = danger.github.pr.additions + danger.github.pr.deletions;
if (linesChanged > bigPRThreshold) {
  warn(`This PR is quite large (${linesChanged} lines changed). Consider breaking it into smaller PRs if possible.`);
}

// Encourage labels
if (danger.github.pr.labels.length === 0) {
  message('Consider adding labels to this PR to categorize it.');
}

These tools can catch issues early in the process and provide consistent feedback.

Balancing Automation and Human Review

While automation is valuable, it's important to find the right balance:

The goal is to use automation to enhance human review, not replace it.

Building a Code Review Culture

Beyond the technical aspects, creating a positive code review culture is essential for team success.

Establishing Team Guidelines

Clear guidelines help set expectations and create consistency:

Example code review guidelines document:

# Code Review Guidelines

## Purpose
Our code review process aims to improve code quality, share knowledge, and ensure maintainability.

## Timeline
- Reviewers should begin review within 24 business hours
- Authors should respond to feedback within 24 business hours
- For urgent PRs, use the "urgent" label and notify the team

## PR Requirements
- Clear description of changes
- Link to relevant issue
- Appropriate tests
- Updated documentation if applicable
- Passing CI checks

## Review Focus
Reviewers should prioritize in this order:
1. Correctness: Does the code do what it's supposed to?
2. Security: Are there potential vulnerabilities?
3. Design: Is the code well-structured and maintainable?
4. Performance: Are there efficiency concerns?
5. Readability: Is the code clear and self-documenting?
6. Style: Does the code follow our conventions?

## Approval Process
- All PRs require at least one approval before merging
- For critical components, require approval from component owner
- No self-approvals

## Communication Guidelines
- Be respectful and constructive
- Frame feedback as suggestions or questions when possible
- For complex feedback, consider real-time discussion
- Use GitHub suggestions for simple changes
- Praise good work

## Resolving Disagreements
- Focus on principles and evidence
- If consensus can't be reached, escalate to tech lead or team discussion

Document your guidelines and keep them in your repository for easy reference.

Training and Onboarding

Help team members become effective reviewers and PR authors:

For new team members, consider gradually introducing them to the review process.

Measuring and Improving

Track metrics to identify bottlenecks and improvement opportunities:

Regularly discuss these metrics as a team and experiment with process improvements.

Recognizing Good Work

Acknowledge and celebrate positive contributions to the review process:

This positive reinforcement encourages continued good practices.

Advanced Pull Request Techniques

Let's explore some advanced techniques to make your pull request workflow even more effective.

Draft Pull Requests for Early Feedback

Draft PRs are perfect for getting early feedback while making it clear the work isn't complete:

Example workflow:

  1. Create a draft PR early in the development process
  2. Add a note about what feedback you're looking for
  3. Continue working while waiting for initial feedback
  4. Address feedback as you go
  5. Mark as "Ready for Review" when complete

Dependent Pull Requests

For complex features that need to be broken down, you can create dependent PRs:

  1. Create the first PR with foundational changes
  2. Create subsequent PRs that branch from the first PR
  3. In the description, note the dependency: "Depends on #123"
  4. Review and merge PRs in dependency order

This approach allows for more focused reviews while maintaining the relationship between changes.

gitGraph commit branch base-feature checkout base-feature commit commit branch ui-feature checkout ui-feature commit commit checkout base-feature branch api-feature commit commit checkout main merge base-feature merge api-feature merge ui-feature

Using PR Templates Effectively

Customize PR templates for different types of changes:

To set up multiple templates, create a .github/PULL_REQUEST_TEMPLATE/ directory with a template file for each type.

Interactive Rebasing Before PR

Clean up your branch before submitting a PR:

git checkout feature-branch
git rebase -i main

During the interactive rebase, you can:

This creates a clean, logical history that's easier to review and understand.

PR Descriptions for Maximum Clarity

Advanced PR descriptions include several key sections:

## Overview
[Provide a high-level summary of the changes]

## Problem
[Explain what problem this PR solves]

## Solution
[Describe your implementation approach and why it's appropriate]

## Changes
[List specific changes, possibly organized by component or file]

## Testing
[Describe how you've tested these changes]

## Screenshots/Videos
[Include visuals for UI changes]

## Performance Impact
[Discuss any performance implications]

## Security Considerations
[Address any security aspects]

## Deployment Notes
[Include any special deployment requirements]

## Related PRs
[Link to any related pull requests]

## Checklist
- [ ] Documentation updated
- [ ] Tests added/updated
- [ ] Accessibility maintained/improved
- [ ] Performance impact assessed
- [ ] Security implications considered
- [ ] Compatibility checked

Adapt this template to fit your project's specific needs.

Advanced Code Review Techniques

For complex PRs, consider these advanced review approaches:

Choose the approach that best fits the specific PR and team context.

Practice Exercises

Let's reinforce what we've learned with hands-on exercises:

Exercise 1: Creating an Effective Pull Request

  1. Create a new branch from main in a repository
  2. Make several meaningful changes (add a feature or fix a bug)
  3. Commit your changes with clear, descriptive messages
  4. Push your branch to GitHub
  5. Create a pull request with:
    • A descriptive title
    • A comprehensive description following the template provided earlier
    • Screenshots or videos if applicable
    • Any relevant labels
  6. Request a review from a colleague

Exercise 2: Performing a Thorough Code Review

  1. Find a pull request to review (from a colleague or an open source project)
  2. Follow a structured review process:
    • Understand the context from the description and linked issues
    • Review the overall change for design and architecture
    • Examine the code line by line for issues
    • Check tests for coverage and correctness
    • Verify documentation updates
  3. Provide constructive feedback:
    • Use line comments for specific issues
    • Use the suggestion feature for simple fixes
    • Ask questions to understand choices
    • Highlight positive aspects
  4. Submit your review with a summary of your findings

Exercise 3: Addressing Review Feedback

  1. Review feedback received on one of your pull requests
  2. Respond to each comment:
    • Thank the reviewer for their input
    • Address the feedback with code changes or explanations
    • Apply suggestions when appropriate
    • Ask questions if something is unclear
  3. Push additional commits to address the feedback
  4. Mark resolved comments as resolved
  5. Request a re-review when all feedback is addressed

Exercise 4: Setting Up Automated Checks

  1. Choose a repository to set up automated checks for
  2. Configure at least three of the following:
    • Linting (ESLint, Flake8, etc.)
    • Automated tests
    • Code formatting (Prettier, Black, etc.)
    • Code coverage checks
    • Security scanning
  3. Set up a GitHub Actions workflow to run these checks on PRs
  4. Configure branch protection to require these checks to pass
  5. Test the setup by creating a PR with intentional issues

Exercise 5: Collaborative Review Session

  1. Form pairs or small groups
  2. Select a complex PR or open source contribution
  3. Conduct a synchronous review session:
    • Have the author explain the changes
    • Review the code together
    • Discuss alternative approaches
    • Document findings and feedback
  4. Compare this experience with asynchronous reviews
  5. Reflect on what worked well and what could be improved

Further Reading