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.
Lots of reasons!
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.
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.
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...
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.
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.
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.
Then it should be quick to code review! Even a PR that fixes a comment can teach the reviewer something.
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.