Developer Productivity and Code Review

Most software projects involving more than a few contributors end up with some process for having more than one person look at a piece of code before it gets shipped. The specifics of what that process looks like and how well it works seems to vary incredibly throughout the industry, but no matter what the setup is, it always has a substantial impact (positive or negative) on a developer’s productivity.

I’d like to share the system that I’ve seen work well for projects (proprietary or open source) involving a team of full time software developers.

Why Code Review?

There are a bunch of different and orthogonal goals for why teams start doing code review, but here’s an incomplete list so we have something to talk about:

• Avoiding bugs: Having a second pair of eyes reduces the likelihood of introducing bugs.
• Redundancy: For any given piece of code, there is more than one person with at least some knowledge of what it does and how it works.
• Training: Code reviews can serve as a structured way for more experienced developers to share their knowledge and give feedback to their less experienced or new-to-the-codebase colleagues.
• Enforcing best practices/rules: Whether it’s enforcing a style guide, requiring automated tests, or making sure third party code has been approved by the legal department, code reviews act as a stop gap for enforcing rules that might be hard to codify with automated checks.

Each of the above goals can be accomplished through means other than code review, but code review still serves as a great way to augment whatever else you are doing to accomplish these goals.

Effective Code Review

It’s easy for teams to start reviewing code, but it can be hard to make the process effective. Here are some traits of effective code review processes:

• Nobody is blocked from doing work while waiting for their code to be reviewed.
• Reviewers do not groan every time they are given code to review and do not feel burdened by having to do it.
• Team members do not see see code review as bureaucratic rubber stamping.
• Reviews catch bugs not covered by other testing systems.
• Reviews trigger technical discussions about best practices.

One way to achieve the above results is for teams to follow a few guidelines to keep things moving efficiently.

#1: the smaller the better

Each diff/pull request being reviewed should consist of a relatively small amount of code. 100–200 lines of code is about right. In some circumstances, you might need to go up to 500 lines of code, which is acceptable but not ideal. Above 500 lines of code, the quality of code review starts to drop significantly. Above 1000 lines of code and I simply won’t participate in a code review process until the diff gets broken up.

Reviewing diffs is hard work but doesn’t provide the same endorphins as writing the code itself. So it’s easy to get “reviewer’s fatigue” when diffs are too long. Sometimes this results in reviewers procrastinating for a long time before actually reviewing your code. Or they just get cranky. Best case, they start cutting corners on their review and gloss over huge swaths of code, which kind of defeats the purpose of the code review.

#2: the sooner the better

When code sits in review purgatory for a long time, a number of things happen which are strictly bad for your team:

1. Code rot. When a lot of people are working quickly on the same thing, you end up with diffs that depend on moving targets. Even if the eventual review indicates that the diff is perfect in every way, there is a high likelihood that you will now end up in merge conflict hell. Or the APIs you were calling have changed and your diff will be dead upon arrival.
2. Increased debt. People usually don’t write bad code out of malice. They usually write bad code because they don’t know any better*. The longer they go without getting corrective feedback, the more bad code they will write, and the more bad code someone will have to review, and the more time will be spent rewriting that bad code.

*or they fundamentally disagree with your definitions for good and bad code, which is a different problem not solved through code review.

If code isn’t getting reviewed within 12 hours of being submitted, you have a problem on your hands. If it takes more than 24 hours, you have a serious problem. And really you should be shooting to get a review within 1–2 hours of submitting the diff.

#3: the more often the better

I like to say that “a diff a day keeps the boss away” (or the PM, or the product owner, or whoever cracks the whips). Unfortunately, despite how much this adage helps me remember to keep sending out diffs, it doesn’t really describe why sending out diffs often is important (hint: it’s not for your boss). And it’s not so much that sending out diffs often is good, but rather that sending out diffs infrequently is a sign that something is probably wrong.

If at the end of the day I realize that I haven’t sent out a diff for review, then usually one of the following is happening:

1. I’ve been super productive and written 2000 lines of code. Oops! Nobody is going to review that diff and I should really stop now and break it up before it gets even longer.
2. I haven’t been able to break down the problem I’m trying to solve into bite sized chunks, which implies that the solution, if I have one, is going to be overly complex, the code is probably going to be spaghetti, and I’m going to have a heck of a time explaining to someone else (let alone myself) why it works. I need to take a step back, try to understand my solution better, do a bit of refactoring, and submit some smaller incremental diffs.
3. I’m having a hard time solving this problem and I would probably benefit from some help. It’s time to swallow my pride and reach out to my colleagues. This might actually mean submitting a diff with what I have so far and asking for feedback. I really don’t want to spend too much time going down the wrong path if I don’t have to.

#4: the more automated the better

Reviewing someone’s code to make sure they formatted it according to the style guide gets old really quickly. Code review comments like “indent” are a waste of everybody’s time. At the same time, it’s actually important that people do follow the style guide and other best practices that your team has established. So how do you make sure people are doing things the “right” way without wasting your time being a nit picker?

The answer is to make it easier to do things the right way than to do them the wrong way, and then go one step further by automating it. I think this principle is best described with an example.

Take JavaScript. A lot of languages have fancy linters that will check for all kinds of problems. Some of those linters will even fix those problems. But JavaScript has a tool that’s really a game changer: prettier. It’s a pretty-printer that parses JavaScript and then prints it back out directly from the AST. And it does so in a way that conforms to a very specific set of formatting rules. By running your code through prettier, typically from inside your code editor, you can fix all the formatting issues instantly.

Prettier is so effective as a formatting tool, that I don’t even bother trying to write properly formatted JavaScript anymore. It’s faster to type bad code and reformat it with prettier than it is to write good code. And when I submit code for review, nobody needs to spend the time writing “indent”. It turns out a lot of people at facebook where the prettier project originated feel the same way:

Eight months after introducing it, 75% of the codebase has been converted and it was all organic.

— Christopher Chedeau

While it can be difficult to decide if it’s worth spending the time to implement any automation system, I find that teams tend to grossly underestimate the benefits of automated code review tools. When you consider the cost of engineering time and the value of good code reviews, automating the basics becomes super valuable.

#5: it’s all about the giff review

Everybody needs a bit of levity in their work day, and code reviews are a great opportunity to do that. Sometimes I relish the chance to do some code reviews just so I can go hunting for the best memes to support the message I’m trying to send. Here’s one of my favorites:

Or when somebody has written something really phenomenal that takes things to the next level of awesome, I just sign off with this:

I have to give special thanks though to my friend and former colleague Alexey Spiridonov who introduced me to giff review culture by leaving this gem on the first diff I ever submitted for review:

p.s. If you liked some of the ideas in this post, give it some claps and follow me on medium, where I’ll be sharing more thoughts on software engineering, startups, and my new company Nori.

p.p.s. At Nori, we’re building a carbon removal marketplace to help reverse climate change. And we’re hiring people in Seattle who like to write software. Send me a message at paul2@nori.com if that sounds interesting to you.