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
presentationdiscussion 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
- Straight read through of the code, line-by-line.
- Focus on security, coding standards, and best practices.
- Hashtagged my feedback: #needsfix, #wouldbenice, #optional, #question
- Over-communicated, adding examples and links as much as possible. Some of my comments, as you’ll see, are better than others.
- 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.
Escape data on output.
Put secrets in a config file.
Best practices
Prefix everything to avoid collisions in global scope.
Remove or comment on dead code.
Considerations
Sessions don’t work well in multi-server environments.
Use WP_Http API for remote requests.
Questions help the developer justify their decisions.
