Building a Code Review Culture

Jonathan Desrosiers
@Desrosj

2016

Information Services & Technology

External Affairs

External Affairs

Marketing & Communications


 


Interactive Design

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 improvements.

Expansion

Need To Fix

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

Planning

Challenges

1. Many concurrent projects at a time.

2. Developers and designers 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.

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. All opinions should be documented in detail with reasoning on the PR.
  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/wordcamp-orlando-2018

Made with Slides.com