Code Reviews at Zymergen
How to be the best dang code reviewers and reviewees possible
At Zymergen, we take our dedication to code quality seriously, so it should be no surprise that code reviews are an important part of our process — every line of code we write gets reviewed. But anyone who has been coding for a while knows that code reviews have a reputation for being a bit of a mixed bag. When done well, code reviews can be quick, good-natured, and a useful tool for catching issues early. When done poorly, however, they can be slow, useless, and a great way to ferment ill-will with your co-workers. Nothing is worse than spending days on a 10,000 line pull request arguing about variable names.
So what is a software team to do? How can we maximize the good and minimize the bad? For Zymergen, the answer has been to create internal guidelines and training on both why we do code reviews and how we do them, which we share with all new members of the team. In this blog post, we are going to look at some of the more broadly applicable parts of that training. We’ll dive more deeply into the motivations behind code reviews and, with a little help from some old friends (more on that later), we will examine how to be the best dang code reviewers and reviewees possible.
Goals and Anti-Goals
Before we dig into the “How” of doing code reviews, let’s take a minute to better understand the “Why.”
First, there are the “obvious” reasons to do code reviews. These are the benefits related to maintainability and correctness that usually come to mind when we think about code reviews. Simply put, you increase your chances of catching bugs early and identifying ways to reduce complexity when you have more eyes looking at the code. More eyes also means more feedback about readability. If a colleague can’t understand a piece of code when reviewing it, they are unlikely to understand it when working with it.
A more compelling, although underappreciated, reason to do code reviews is that code reviews help build a team’s “engineering culture.” By that I mean that code reviews can create norms, indoctrinate new engineers, and disseminate knowledge. For example, code reviews are a natural forum for openly exchanging ideas and techniques, and so they are great at organically surfacing new best practices or spreading awareness about existing best practices. Case in point, it is pretty common for new hires to inadvertently write code that follows the style guidelines from their previous job; therefore, their first couple of code reviews are an important opportunity for more tenured engineers to offer feedback and reinforce the current team’s programming guidelines and conventions. Code reviews also spread awareness about how the codebase is evolving and what new capabilities are being built in. This decentralizes knowledge and gives reviewers the context to potentially pick up a feature if the primary developer needs support, or to use what someone else has built for their own purposes.
So those are some things code reviews are good for, but it’s worth mentioning one thing in particular code reviews are not good for. Code reviews are not an effective communicator of high-level design. They are too implementation focused, and the grander vision and wider context of what one is trying to accomplish can too easily get lost. They also do not encourage “failing fast.” By the time someone has posted a code review, they have likely committed themselves to a particular design and it can be difficult to let go of that sunk cost. If communicating design is your goal, favor a design document and a design review process that precedes major coding work. Even better, link to this document when you post your review so that reviewers can get a refresher on the context.
Goofus and Gallant®: Lessons on Code Reviews from Timeless Teachers of Good Behavior
In this section, we’ll explore the types of desirable and undesirable behaviors that impact the overall worth of a code review. We’ll learn about these behaviors by meeting two hypothetical software engineers, Goofus and Gallant®, who approach their responsibilities on code reviews very differently.
If those names sound familiar, it’s because “Goofus and Gallant®” is the name of a long-running children’s educational comic strip appearing in Highlights magazine. The comic teaches kids how to act virtuously by comparing the actions of the two main characters — aka Goofus and Gallant®. Goofus doesn’t always think before he acts; Gallant behaves in a more thoughtful way.
The simplicity of the comic and its “explain by example” approach makes it an excellent educational tool; and if it is good enough to teach children how to be respectful and responsible human beings, it is good enough to teach software engineers how to review each other’s code.
Responsibilities as a Reviewer
When reviewing someone else’s code, Gallants make it their number one goal to ensure that a quality product makes it to production in a timely manner. They comment judiciously, and provide clear, meaningful feedback when they do.
Gallants avoid rubber-stamping, giving ambiguous direction, or, toward the other extreme, being overly exacting. These actions are more in line with Goofus behaviors: they don’t add value and do more harm than good.
Responsibilities as a Reviewee
When having their code reviewed, Gallants optimize for making the life of the reviewer as easy as possible. They recognize that doing a review takes a lot of mental energy, especially if the reviewer is not 100% familiar with the topic.
Dealing With Disagreements
Most often the reviewer and reviewee agree on the proposed revisions and it is a piece of cake to tidy up one’s code. This keeps the process moving smoothly and leads to frequent commits of high-quality and narrowly-focused code. However, even when we are all acting like Gallants, sometimes a pull request gets unduly held up. Maybe the reviewer and reviewee have differing opinions on what’s a critical suggestion versus a nitpick. Or perhaps the reviewer and reviewee both agree on a problem but have trouble agreeing on a resolution. Disagreements happen, so what actions can one take when they do?
The first step is to adopt the correct mindset. One of Zymergen’s core values is to “Assume Good Will.” This philosophy makes any disagreement easier to hash out because it encourages an open-mindedness to the other party’s suggestions. When one does not assume good will, it becomes more difficult to listen without judgement and communicate without friction.
The next step is to move the discussion to in-person. It is easier to express nuanced ideas and quicker to iterate through different perspectives when engaging in direct, face-to-face conversation than when exchanging emails or thread comments. It can take minutes to craft a response in writing that would otherwise take seconds in front of a whiteboard.
If there is still a disagreement, another option is to bring in a third party. An outside perspective can help highlight issues that might otherwise be overlooked or undervalued, and can offer additional solutions that have yet to be considered. Third parties also have the luxury of emotional distance from the disagreement and so can more objectively weigh one solution over another.
Ultimately, if a consensus still cannot be reached, Zymergen’s rule is that deference goes to the implementer. We trust our engineers to make the best decisions given the information they have available to them and to own any decisions they make. Plus, sometimes the only real way to validate whether an idea is good or not is to just merge it, see what happens, and remember that `git revert` is a quick path to absolution.
Wrapping Up
Zymergen’s approach to code reviews has worked well for us when measured against the benefits in the “Goals and Anti-Goals” section of this article: We are finding bugs, improving maintainability, building an engineering culture, and all the while keeping everyone working well together. The key is that reviewers and reviewees alike know the roles they must play when entering a code review, and they have clear direction on how to play those roles as best as possible. If a code review begins to stray into the realm of unproductivity — which inevitably happens, since we cannot precisely define perfect behavior in all scenarios — engineers are encouraged to take the discussion offline to get things back on track.
I hope you have found this advice helpful when considering your own approach to code reviews, but I want to leave you with one final thought. If you really want to get the most out of any process (code reviews included), the most important thing you as an individual can do is to model good behavior. When you act with intention, you inspire others to do the same. Conversely, if you take a halfhearted approach and engage in bad practices, those actions will be mirrored back to you. To put it in terms of our old friends Goofus and Gallant®: Gallants invite Gallants, and Goofuses end up surrounded by more Goofuses. So do yourself a favor…be like Gallant.
Zach Palchick is a Senior Software Engineer on the Computational Biology team at Zymergen.
Special thanks to Highlights for Children for permitting us to use Goofus and Gallant® in this post.
