How I review code

cyle:

Reviewing code is one of the most important parts of an engineer’s job at Tumblr, even more so than writing code. Our codebases are shared by hundreds of engineers, so it’s critical to make sure we’re not just writing the best code we can, but that the code being written can be understood by others. Taking the time to review someone else’s code is the most critical opportunity to ensure all of that is happening.

At Tumblr, every code change happens in a Pull Request on an internal Github instance. We have repositories for the PHP backend, our database schemas, our iOS (Swift/Obj-C) and Android (Java/Kotlin) mobile apps, infrastructure projects written in Go, C/C++, Lua, Ruby, Perl, and many other projects written in Scala, Node.js, Python, and more. All of our code repositories rely on authors to write Pull Requests and get approvals from their peers before merging their changes to the master branch and deploying to production where real people interact with it.

How I personally review code has changed considerably over my few years at Tumblr. Before working at Tumblr, I wrote code mostly by myself and reviewed code with a very small set of people. Shifting to a huge codebase with hundreds of contributors was a big change. Thankfully I’ve had some good teachers. I went from reviewing maybe one pull request a month to currently reviewing an average of 25 pull requests a week. Here are some of the principles that help me keep my reviews timely and helpful.

Review the code with its author in mind

The first thing I ask myself after a review has been requested of me is who wrote this? Are they a junior or senior engineer? Are they new to this codebase or a seasoned veteran? Have I ever reviewed their code before? Am I familiar with the project this code change contributes to?

When I’m reviewing the code of someone I work with closely, I probably know pretty well what their thinking was when they wrote it, and I have an idea of what experiences they’ve been through. Junior engineers sometimes need a little more hand-holding, which usually means giving them more help with code examples and references. Senior engineers sometimes need to be reminded that highly performant, abstract, or clever code is often difficult to read and understand later, which usually means asking them for more inline comments and documentation.

It’s also fundamentally important to review the code as if anyone could read the review you’re about to submit, not just the author. There are two main reasons for this. First, some people learn by reading the reviews that other engineers write; as a more junior engineer that’s exactly how I found out the most about the intricacies of Tumblr’s codebase. Also, in six months’ time it’s very likely you may be looking at this code again to figure out how it works. Having a helpful code review of it around can give some insight into the decisions that went into why it works the way it does.

Review the code with everyone else in mind, too

The core of my review, no matter who is writing the code change, centers around being able to understand the code itself and the motivations and context around it. To me, ideally anyone should be able to pop into a pull request and expect enough context to understand the code change and why it was done the way it was done and how it works the way it works. This is especially important in an old, shared codebase, where someone three years from now may be looking at your PR to figure out why you chose to do what you did. If that’s not included, or if there aren’t at least links out to the relevant context, something is wrong. More detail is always better.

I don’t worry as much about code style or syntax itself, as we have automated processes to ensure that new or changed code conforms to our agreed-upon coding standards. Similarly to what I wrote about in how I code now, I look for code that is well-documented (both inline and externally), and code that is clear rather than clever. I’d rather read ten lines of verbose-but-understandable code than someone’s ninja-tastic one-liner that involves four nested ternaries. Especially if the person writing the code has been around the block a few times and been burned themselves by old, undocumented, clever code.

Once I feel like I can understand the code change, I try to put myself in the shoes of someone who doesn’t deal with this area of the codebase very often (which may be the case for me at the time!) and think of how to review the code to help make it clear for them. I try to think of someone new being hired six months from now, looking at this code, wondering how it works.

Understand the PR’s scope

Sometimes not everything can get done in one pull request. At Tumblr we try to keep our PRs small so they can be reviewed quickly and safely, rather than bundling a ton of hard-to-review work into a 5,000-line-change PR. Because of this, sometimes the work has to be broken up into chunks, with PRs that build a foundation and lead to future PRs with finished implementations.

Or, alternatively, it’s common for evergreen codepaths to have known issues or work that’s been ticketed for future sprints, so it’s become a good, common practice to leave a @todo in the code with the name of the ticket where that todo will get done. That way we can unblock code changes from having to be totally complete within one pull request.

Stay on top of the whole review process

The number one thing that helps me review code in a timely manner, and stay on top of updates about PRs, is email. I check every Github email I get; I make sure that I don’t get notified for everything that happens in the repo, but I do get every email that happens relating to a PR I’m associated with. This helps me stay on top of every step in the review process, because it’s almost always a back-and-forth that ideally shouldn’t last more than a day.

At Tumblr, most of our reviewers are selected by automated round-robin assignment when the PR author is ready to receive reviews. That assignment triggers an email and subscribes me to everything that happens relating to that PR. From there, it’s on me to stay on top of my email and make sure that I not only allocate time to do the review as soon as possible, but follow up on the PR if I leave a review and the author updates it in response to my review.

Remember to be a human

The most important advice for reviewing code (and, in other ways, writing code) is to remember to be a human. Remember that the person who wrote the code you’re reviewing is also a human. Give them the benefit of the doubt. Be nice when you write a suggestion, or have a question, or find an edge case that they don’t seem to have covered. Even if they’re a seasoned veteran coder who has written bulletproof performant code for years, treat them like a person who makes mistakes sometimes. Even if they’re someone you work with every day and you feel comfortable cracking jokes at their expense, understand that a new person might not understand.

Remember that shared, living codebases are often hectic and strange, especially ones that have been around for a decade. Remember that sometimes things are in a rush, so you can only do the best you can. We can’t halt everything in the name of perfect code, but we should make sure that everyone is doing the best they can, whether we’re writing or reviewing code.

CAN imagine review a PR with 5000-line changes. 😭