How We Review Code at Canopy

By Joe Delfino on May 01, 2019

Over a year ago, we sat down at various Central Square establishments to talk about all the ways people screw up engineering culture and how we were going to get it right this time (this is now our official unofficial engineering motto). We talked and talked, we wrote it all down, and then we let that doc sit, because getting it right is not something you can do by fiat.

A year later we’re most proud of the way we review each other’s code. That probably sounds mundane but it’s a crucial first step in addressing how we work together as a team. Taking the time to read each other’s work and respectfully ask questions has set the tone for how we build things here. Having walked the walk, we deemed it safe to write down and share our review philosophy. It’s helped us a ton, and we hope you find it useful.

Q: Why do we do code reviews?

Lots of reasons!

  • To let a reviewer learn about the change, so that more than one person knows about it.
  • To help both the coder and reviewer learn how to write better code - language features, design patterns, testing strategies, conventions, etc.
  • To help catch bugs or oversights.
  • To have a second person think about test coverage for the change, and suggest ways to improve it.
  • To identify confusing parts of the change that should be documented or refactored.

Explicit non-reasons:

  • To let the author and/or reviewer feel smart.
  • To let someone else catch/be on the hook for bugs you wrote.
  • To nitpick style.

Q: Should code reviews be supportive?

We all know this is a gimme question, and the answer is a loud YES! Code reviews are not a time to look smart, or make another person feel bad about a mistake or oversight. Remember that there is a human on the other end of the code review, and they worked hard on whatever change you are looking at. In response, we work hard to be thorough, thoughtful, kind, and professional when reviewing our colleagues code.

So be nice, supportive, and encouraging in your comments. When making style suggestions, offer them as opinions, not facts. Don’t focus on style to the exclusion of understanding what the change does.

Q: Wait, first you said no style nitpicks, then you talked about making style suggestions. Do style comments belong in code reviews, or not?

There are style comments, then there are style comments. Style can be very important to readability and simplicity of code. Small things like a confusing name, or deviating from a convention can make new code confusing to read.

If you, as a reviewer, are confused by something in a PR - even if you can figure it out yourself by poking around in the change - you should feel totally comfortable making suggestions to improve readability. Chances are, someone else reading the code later will have the same confusion. These suggestions might include asking for more comments, asking for variable renames, or suggesting alternative factorings or simplifications.

As an author, you might find yourself thinking “that’s silly, it’s obvious,” or “I like my way better.” You’re probably even right sometimes, and should push back on the comment if you feel strongly. But, keep in mind that code is read many more times than it is written, and you should give your reviewer’s opinion extra weight because it represents the majority who will read the code but did not write it.

On the other hand, as a reviewer, if an author writes code in a style that is not your favorite, but it doesn’t noticeably impact the readability of the code, then you can just grumble to yourself and move on with the rest of the review.

Q: Can a reviewer suggest big changes?

Yes! Up to and including “I don’t think we should make this change.” However, suggestions for big changes should happen as conversations, rather than statements of fact.

For example, “I have this idea for a big change that I think would make this better, let me explain…how does that sound? Do you think it makes sense to do that now, or later?” is better than “Here is a better way to do it: …, let me know when it’s ready to look at again.”

The timing of those big changes should also be a discussion. See the next question...

Q: Are code reviews a gate?

Yes and no. Every change to master should be code reviewed. Beyond that, it is the responsibility of the author and reviewer to agree on what feedback needs to be addressed before the PR is merged.

As a reviewer, it’s helpful to be explicit about what changes you think should be addressed before the PR goes in, as an immediate follow on, or some time in the future.

As an author, if you are inclined to skip or defer a piece of feedback, it’s expected that you’ll reply to the feedback with a reason for deferring or skipping it.

Q: I’m reviewing a change that is blocking a release or some other time sensitive thing, what should I change?

You should give the change the same level of scrutiny, and leave the same comments you would otherwise. However, you may want to be more flexible in terms of which pieces of feedback you consider to be blocking vs. which can be addressed in a follow on PR. For example, you might explicitly note that style can be addressed in a follow on PR so that the functionality can land sooner.

A looming deadline is never an excuse to ignore quality, readability, or test coverage. It may be an excuse to defer addressing some of it in a particular PR though.

Q: When should I request a review?

For larger changes, getting an initial review early is often really helpful - if the reviewer suggests big structural changes, making them before polishing the rest of the change can save a ton of time. On the other hand, reviews do take time, so you don’t want to ask for one every time you push a single commit.

If you do ask for an early review, make your expectations and the state of the change clear to the reviewer. For example, saying “I don’t have tests yet, and I need to do a naming pass, but can you take a look at the class factoring I have here?” can save the reviewer a lot of time.

Q: What if something is really trivial - does it need a review?

Then it should be quick to code review! Even a PR that fixes a comment can teach the reviewer something.

Q: What if something is on fire and I can’t wait for a review?

We’re all adults, use your judgement! If you do have to submit something without a review, get a post-submit review as soon as you can.