#pdxwp: Code Review Takes Two

We did a second pass at our code review meetup — last night turned out much better than the first. The high point for me: most of the “presentation” was, in fact, discussion. The latter proved to be way more valuable for everyone, as most of the twenty people in the room don’t do code review on a regular basis.

Here’s what we did:

  • Jeremy Ross submitted a section of code he had been working on, along with instructions on what he wanted feedback on.
  • On Saturday, I reviewed the code. I committed it in one commit to a branch in a private Github repo. On the changeset, I did a line-by-line read-through, commenting as I went. To wrap the review up, I created a pull request explaining how I did the review, what I looked for, and how to interpret my feedback.
  • Saturday night, I prepared a reveal.js presentation with an introduction to code review and the contents of what I found in my review. reveal.js is a super slick tool for preparing a HTML/CSS/JS presentation out of content in Markdown.
  • Jeremy read and considered my review, then updated the presentation with his feedback.
  • I did most of the presentation discussion facilitation, and Jeremy talked through how he received my feedback.

reveal.js doesn’t produce great static slide output, and I used its Markdown feature which requires Grunt to serve, so the bulk of what we covered will forever live on in the outline that follows.

What is code review vs. a code audit?

Code review is a process for testing, revising, and confirming code before pushing to production.

Code audit is a similar process, but happens after the push to production.

Why is code review useful over code audit?

There are a number of reasons, primarily…

  • Promotes small, frequent improvements over large changesets. With the former, it’s easier for the reviewer to confirm the code change matches the expected behavior change.
  • Easier to change direction on the architecture of the code before pushing live than after pushing live.
  • Developers have greater incentive to fix problems before they’ve pushed to production, than after.

How do you do code review well?

Well, everyone has their opinions. Here are some:

  • Establish guidelines for the review: what’s being reviewed, what the reviewer is looking for, and how feedback will be provided.
  • Reviewee should include details on how to test anything that needs to be tested.
  • Reviewer should over communicate, prioritize feedback, and be constructive, not adversarial.
  • Code review is a conversation. Timeliness, courtesy, and clarity all matter.

How Daniel reviewed Jeremy’s code

  1. Straight read through of the code, line-by-line.
  2. Focus on security, coding standards, and best practices.
  3. Hashtagged my feedback: #needsfix, #wouldbenice, #optional, #question
  4. Over-communicated, adding examples and links as much as possible. Some of my comments, as you’ll see, are better than others.
  5. Didn’t QA code against what it’s supposed to do, or give much thought to how you could restructure it.

Issues Daniel found in the code

Security

Validate / sanitize data in input.

Sanitize user input

Escape data on output.

Escape data on output

Put secrets in a config file.

secrets

Best practices

Prefix everything to avoid collisions in global scope.

Prefix everything to avoid collisions in global scope

Remove or comment on dead code.

Remove or comment dead code

Considerations

Sessions don’t work well in multi-server environments.

Sessions don't work well in a multi-server environment

Use WP_Http API for remote requests.

Use WP_Http API for remote requests.

Questions help the developer justify their decisions.

Questions help the developer justify their decisions

Author: Daniel Bachhuber

Proud father x2. Principal, Hand Built. Maintainer, WP-CLI.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s