Code Review: When your code becomes our code

This is part one of a two-part piece on code review that I shared with my colleagues. The next part will focus on code review etiquette and provide an appendix of relevant resources.

Code review is a required step of the development process at my company. That said, as I finished reading through a recent PR, I found myself wondering if we provided any in-house guidelines about how to do it. Searching our internal wiki I found only this write up of our official policy:

Code doesn’t get merged until another engineer can review the change. Those that are dissatisfied with the content of the PR leave feedback for the author to correct. When reviews are satisfied by another engineer, commits are then merged and the source branch removed.

This is an accurate description of our process but the term satisfied didn’t offer any specifics as to what to expect as a code author or what to do as a reviewer. What follows is what I shared with my coworkers in an attempt to offer my own insight into why we do code review and how the process should work.

Code review happens before your code becomes our code

Pasteurizing the milk

Code review is the final step before your code becomes our code. The code that you wrote and understand becomes code that the reviewer and others will need to understand, maintain and likely modify. Generally desirable properties for code that we will collectively own can be captured under the term “code quality.” Obviously the code also needs to work; that property can be described by the term “code correctness.” Maximizing both quality and correctness are the primary goals of a code review.

The quote at the beginning describes ensuring that the reviewer is not dissatisfied with the change. This suggests a directional nature to code review: that one party can satisfy the other. Apart from quantitative concerns about quality and correctness, what I hope for from code review is that the code author and the code reviewer should both feel good about making this change to the repository. While this is not always possible and it may involve a bit of compromise, I believe it should be possible in most cases and it is the responsibility of both parties to try to come to this place.

Code Quality: Equal partners

New code that is merged into a source repository becomes representative of the standard of quality in that source repository. Code review is the chance for you to raise or lower the bar for readability and maintainability. Because of all the “externalities” of merging changes into a repository, I view the author and reviewer as equal partners in the quality of these changes. While you may feel you should be generous in a code review out of a sense of politeness to one of your coworkers — who you like and with whom you want to remain on good terms — the fact is that their code now becomes code that you have to maintain. You shouldn’t take this responsibility lightly any more than you’d allow someone to borrow your bicycle and return it with both tires flat. Personally, I will ultimately defer to the code author except in exceptional circumstances, although I can’t say with confidence this should be a given; I just find it minimizes conflict.

Some organizations have explicit rules about code quality. At a minimum, ensuring some consistency with accepted patterns in the codebase is important for readability. Pattern recognition can save the human mind a lot of time. A reviewer may also have greater scope than the author and may recognize areas of redundancy in the codebase. Finally, code authors get locked into their way of thinking as they work through a problem and a fresh perspective may reveal decisions that may not have been given but became baked in to the code author’s mind during the development process. Beyond the above, recommendations for improving quality may vary by reviewer. No matter what recommendations are offered, being “equal partners” entails that as a reviewer you will invest the time needed to help the code author achieve the goals you put forth in your review.

Code Correctness: Less equal partners

While you can contribute to code correctness in a code review, I feel your contributions in this regard may be more limited. Some cultures feel differently. Yelp’s model is that “You are equally as responsible for the code shipped as the person who wrote the code.” At companies that pair-program, it is, of course, implicit that both authors responsible. At my own company, the policy is that ultimately you, the code author, are responsible for the correctness of your own code.

Code review can definitely find stupid mistakes, such as that “Printf” for debugging that you left in the code. It can catch usage of deprecated methods and known — but unknown to the author — security and performance problems by bringing in greater scope of knowledge. I personally am dubious that as a reviewer you can efficiently and effectively build the kind of in-depth mental model needed to dig much deeper on corner cases than the code author. The brain goes through a different process writing code than reading it. For example, there are a lot of options you considered and excluded when you wrote the code; the code reader may never see and explore those dead ends. That doesn’t mean that as a reviewer you won’t find something deep on occasion or that it’s not worth looking; however, I think expecting the reader to truly understand code as well as the original author is a false hope.

The process of reviewing code for correctness can be broken down into a number of specific activities, listed below in increasing level of effort:

1. Ensure that there are tests.
2. Make sure you understand every line of code completely.
3. Review each individual test case and make sure that you believe all corner cases are covered.
4. Fetch the branch and run it locally in your own development environment.
5. Try to break the test cases locally and make sure they fail as expected.

What actually happens during Code Review

I’m confident that reviewers at my company don’t follow every step in the list above, nor do I believe it’s always desirable. I’ve personally not even made it as far as step 2 at times, and occasionally I forget about step 1 (ideally step 1 is automated). Reviewer “fatigue” is known to set in with changes that involve hundreds of lines of changes. I’m not ashamed to say that I do relax my guard about code that may not be mission critical. I also tend to lower the bar for code quality for test code. Finally, for big changes, I find it very difficult to meaningful verify that test cases cover all corner cases. Note that when developing with TDD (test-driven development) you don’t need to do this — you will have the minimum implementation that exercises each test case — so closely reviewing tests isn’t a habit I personally picked up while I was doing TDD. Ensuring complete test coverage may also not necessarily be a requirement for the reviewer if they are not burdened with shared responsibility for code correctness. Hopefully, a code coverage tool can warn you if anything big is not covered, although it will not identify all corner cases.

Other goals of Code Review

There are few other reasons we do code review that are not check-box items for either party but are still worth mentioning:

1. Knowledge sharing. Code review is a chance for both parties to learn from each other, both about how to write better code and about the architecture of the system.
2. Avoiding siloing. More than one person in the company will now have familiarity with this code.
3. Socialization. The more we interact with each other the better we will function as a team.

Keep in mind these benefits when you go through the code review process as an author; this process is more than just another lint phase for your code.

A Final Note on Automation

Maximizing the amount of automated checking before the review will minimize time and friction in the review process. There are a number of ways that automation can help:

1. Run all the linters you can.
2. Code complexity analysis tools can provide a starting point for refactoring, although their metrics don’t necessarily favor readability.
3. A checklist on a Github PR template can help guide both code reviews and authors towards providing the best possible review feedback.

Now check out part two, etiquette guidelines.