Reviewing code

As a former Program Technical Lead for OpenStack, part of my duty involved identifying and supporting "core reviewers" from our open source community: developers believed to be capable of upholding high technical standards and thus empowered to drive the project's direction via gerrit. While each reviewer always brings a differing perspective and contributes their own merit to the project, I've learned to recognize a common set of values that I see in every great reviewer almost regardless of technical experience or background.

So, if you're looking to spend more time reviewing code and be more effective doing so, I highly encourage you to always keep these prevailing goals in mind:

  1. Prevent bugs. As a reviewer expressing support for a change, you are accepting equal responsibility for the consequences of the change. When your peers understand and agree with your code before it merges, you're much less likely to suffer from unexpected bugs. And when you do, your team will be able to share responsibility for the breaking change and have the requisite knowledge to quickly produce a fix.

  2. Maintain a cohesive direction. As a reviewer, you should have oversight over all proposed changes coming into the project and be able to effectively judge the merits of multiple conflicting solutions to the same problem.

  3. Help other contributors. As a reviewer, it is essential to openly communicate with contributors and provide them with the critical feedback necessary to understand why their changes do not meet your expectations. Even more importantly, you should work with authors in an effort to improve their future contributions. If your criticism of a patch is just nit picking, you should contribute a new patchset to fix those problems yourself.

  4. Avoid accruing technical debt. Is the change tested, documented, and elegantly written? Those problems are relatively easy to identify and fix. On the other hand, does the change create an overly tight coupling or expose an high-maintenance API? It's much more difficult to identify those types of issues before they become costly.

  5. Consider changes objectively. As a reviewer, it's easy to succumb to bias when reviewing code. When you stand to personally benefit from a contribution, you increase the likelihood of sacrificing review quality. Conversely, subjective reasoning might lead you to prematurely oppose a change, disregarding its value.

  6. Ask questions. Reviewing code is a two-way conversation, and that means everyone has an opportunity to learn. Challenge the author to explain their work when the motivation or direction is not obvious. Consider the need for better inline comments, documentation, or commit messages when such explanations may benefit others in the future.

  7. Be nice. Recognize and respect the time commitment and emotional investment other contributors have put into their work. Provide positive feedback and encouragement in addition to criticism. Treat others like friends and family, not subordinates.

We're all imperfect, and there's certainly a balancing act to perform here, but I hope you can take these to heart on a daily basis.