Having fun working on solo projects. Otherwise doing data science, data engineering and web development.
There's a ton of resources scattered around the web dealing with code review fundamentals, best practices, tools, etc. In this article we'll summarize the lessons from several official engineering blogs of well known tech companies.
We'll cover several topics:
The topics are covered fairly independently, so if you're curious about a particular topic feel free to skip ahead.
It should be obvious that the primary purpose of code review is to assess quality of the changes being introduced. I mean, the dictionary definition of review says precisely that
review (noun) - a formal assessment of something with the intention of instituting change if necessary.
Of course, code being code, there's a lot of things that can be checked and tested automatically, so there's nuance to what actually needs to be checked in an actual code review. We cover that in the next section.
On the other hand, code review is a form of communication between the author of the change (these days usually a pull request) and one or several reviewers. So it has side effects that go beyond preventing bugs from slipping in or keeping the codebase consistent in terms of style and architecture.
When done well, code reviews help accelerate learning across the team, create psychological safety for all team members, help establish and communicate best practices, teach proper communication and improve team dynamics. When done poorly, they can help deteriorate all of the above.
There are a bunch of ways in which code reviews help maintain the quality bar for the codebase and the product. In the end it comes down to catching mistakes at the level which can hardly be automatically tested, such as architectural inconsistencies. Also, the code for automated tests should be reviewed, so there's a meta level at which reviews help with QA.
In Giving High Leverage Code Reviews, Casey Rollins advocates for having a checklist with all the usual things that need attention.
When I’m reviewing a pull request, I often do multiple “passes” where I focus on one attribute at a time. I start at the beginning and review the pull request with a single attribute in mind before moving on to the next. When I’ve worked through the checklist, I submit the review.This checklist moves from general to specific checks because it’s important to focus on the high-level attributes first. It doesn’t make sense to offer a variable name suggestion if you’re also suggesting that an entire class or function be refactored.
You can have your own checklist or make it a shared list for the team or a project. There's a ton of material written on the usefulness of checklists. In Getting Things Done, David Allen puts forward a simple idea - our minds are great at processing information, but terrible at storing and recalling it. That's why checklists are a great way of externally storing and breaking down a planned or repetitive task.
It's a pretty long list. In addition to it, a recurring piece of advice is not to use code reviews in place of static code analysis tools. If your review is mostly about code formatting, variable naming and alphabetical ordering, it might be a good time to include an automated code analysis tool into your development workflow.
In Effective Code Reviews: Bettering Products, Teams, and Engineers from PayPal engineering, Gabriel McAdams points out several important benefits of code reviews related to team dynamics:
In Code Review Best Practices from the Palantir Blog, Robert Fink lists several ways in which knowledge sharing and social side-effects happen via code reviews:
Code reviews should be seen as a team effort. Once you view them that way it becomes clear that both sides - the author and the reviewers - have their distinct sets of responsibilities.
In this short post on Medium Engineering blog, Xiao Ma describes how a different perspective changes the way code reviews are done, how feedback is taken and how people on each side benefit by adopting a positive mindset about code reviews.
When we talk about the responsibilities of the pull request author, there are several key things recurring in all code review guides.
1. Make pull requests as atomic as possible
At Shopify they advise to keep your pull requests small - it helps the reviewer dive into it and finish it as an atomic piece of work in their workday. In practice this can mean keeping your pull requests limited to a single concern. A single concern here means a single bug fix, a feature, an API change etc.
Don't mix refactoring that doesn't alter behavior with bug fixes or new features. This is beneficial both for the ease of doing the code review but also helps keep the codebase maintainable (for example, atomic pull requests are easier to rollback).
2. Provide a helpful pull request description
"Give your reviewers a map". It's true that you should pick the teammates that are the most familiar with the part of code you've changed. But even a few sentences describing why/what/where of the pull request can greatly help the reviewer to navigate your pull request.
3. Test before review
Make sure you've reviewed and tested the pull request before submitting for review. You want to make sure that all relevant files are included, that the PR passes the build and automated tests, that all suggestions from automated review tools are addressed.
The most frequently recurring piece of advice, and perhaps the least obvious, is the importance of the tone of communication in code reviews.
In a Kickstarter Engineering article A Guide to Mindful Communication in Code Reviews, Amy Ciavolino lists many tips for improving communication on both sides of a code review.
In Amy's words: "Technical skills are needed to review code thoroughly and quickly. But beyond that, reviewing code is also a form of communication, teaching, and learning. Either as the author or the reviewer of code, being mindful in your communications can make code reviews more valuable for everyone."
The article contains tips on how to be mindful of the author and the purpose of the process when doing the review:
A bug is a bug, a typo is a typo and there's no way around it. But even if it's an obvious mistake, there are often multiple ways to deliver the message. A code review ridden with comments like This is duplicate; Fix this...; Feels slow. Make it faster; Read the style guidelines can come as too harsh no matter who the author is.
This is nicely pointed out in Giving High Leverage Code Reviews:
At the core of a code review, you’re providing feedback to your peers, which might be hard. But receiving feedback is harder. Everyone on your team is trying to do their best work, so take care in delivering your message. For example, if you’re pointing out an error or asking a question, make it a team effort, not their fault. This might look like: “Can we remove some of the duplication in this file?” instead of “You missed an edge case”.
Alejandro Lujan Toro offers several practical examples of harsh comments that you can easily change to a more constructive tone:
The trick is to approach code reviews as a team effort. Try to use more we instead of you when suggesting changes. Amy Ciavolino suggests that you shouldn't even start reviewing if you're not in the right mood to give considerate feedback:
When you’re checking in, also consider how you’re feeling in general. Giving kind and considered feedback takes time and energy. If you’re hungry, tired, in a hurry, have a lot of meetings, etc., don’t review code or send out a review. You need to fix those things first. If you don’t care for yourself, you can’t take care of others.
Once you realize that code reviews are not simply about finding bugs, this should come naturally. Maybe you've learned something from the pull request, or the author has invested a great effort and showed impressive attention to detail. Let them know that.
Giving praise in code reviews is especially important with newcomers. In How to Make Good Code Reviews Better, Gergely Orosz suggests that code reviews need to be a positive experience for a newcomer:
Better code reviews pay additional attention to making the first few reviews for new joiners a great experience. Reviewers are empathetic to the fact that the recent joiner might not be aware of all the coding guidelines and might be unfamiliar with parts of the code.
These reviews put additional effort into explaining alternative approaches and pointing to guides. They are also very positive in tone, celebrating the first few changes to the codebase that the author is suggesting.
That concludes this summary of best practices for code reviews. You can find links to the original articles in this Blogboard search. Blogboard helps you search, discover and follow official blogs published by leading tech companies.
Previously published at https://blogboard.io/blog/code-review-best-practices/
Create your free account to unlock your custom reading experience.