Building a Successful Code Review Culture

Jonathan Desrosiers
@Desrosj

2016

Information Services & Technology

External Affairs

External Affairs

Marketing & Communications


 


Interactive Design

Clientele

Interactive Design

Jonathan Desrosiers

Web Developer III

First Assignment

Findings

  • Inconsistent code style.
     
  • Some missing best practices (uncached function calls, missed output escaping, etc.).
     
  • Performance improvement suggestions.

Expansion

Need To Fix

  • Inconsistent code style.
     
  • Some missing best practices (uncached function calls, missed output escaping, etc.).
     
  • Performance improvement suggestions.

Goal

Establish a structured mandatory code review process to enforce consistency, improve code quality, and promote learning, knowledge sharing, and collaboration.

Challenges

1. Many concurrent projects at a time.

2. Not just developers write code.

3. Lots of variation in skills and experience.

4. Some projects are very old or long term.

Technical Factors

Human Elements

1. Opportunity To Learn

2. Opportunity For Knowledge Sharing

3. Collaboration

4. Improved Onboarding

Steps

  1. Establish Rules
  2. Create a Process
  3. Document the Process
  4. Train Everyone

Step 1:

Establish Rules

1. Everyone must use the same code style.

2. Everyone must follow best practices.

3. Everyone must follow the same workflow.

4. Everything must be properly documented.

5. Everything must be accompanied by tests

(if appropriate).

Step 2:

Create a Process

What is the correct way to GIT (for us)?

Our GIT Flow

  • No committing directly to the master branch.
  • All code must be added through a pull request.
  • Primary Branches must always be "production ready".
    • Plugins: `develop`
    • Themes: `master`
  • Merging code to `master` when not the primary branch requires a PR.

Our GIT Flow: Plugins

  • When a new version is ready, a PR from develop to master should be opened that:
    • Increments the version number.
    • Updates the changelog.
  • Create a "Release" in GitHub

What Are Our Coding Standards?

Differences

Continuous Integration

How Do We Enforce Code Style, Best Practices, and Standards?

Step 3:

Documentation

Every Pull Request

Step 4:

Training

BETA Testing

Additional Pull Request Rules

Limit PR Scope

Include a URL to Test

Avoid LGTM
(Looks Good To Me)

Make Suggestions

Delete Branches After Merging

Good Branch Naming

  • 12345-add-missing-sanitization-for profiles
  • fix/broken-admin-links-for-members
  • release/5.0

PR Responsibilities

Stalemates

Stalemate Rules

  1. Document all opinions.
  2. A third party will be called in to make a final decision.
    1. Which option is the most "futureproof"
    2. Does one approach prevent pivoting later if the chosen approach does not work?

Code Review Etiquette

Review Code Not the Person

Avoid You

Engagement

Additional Success Factors

Everyone

must buy in.

This is now a required part of the process.

Clearly Set Expectations

Everyone Reviews Code

Bonus:

Additional Processes

Pull Request & Issue Templates

Automatic Deployment to Staging

Visual Regression Testing

Learning

Everyone. Makes. Mistakes.

Thank You

Twitter: @Desrosj

 

https://jonathandesrosiers.com/wcus

Made with Slides.com