My Code Review Journey

By Jonathan Stanton

You have just finished up with a big feature. You smile to yourself as you look over your hard work and then spend a few minutes writing a short description before you send it out to a co-worker for review, but the sheer size of your change gives you pause.

“Uh-oh” you say out loud “The diff is huge, with thousands of lines of code changed” With a tinge of guilt and a resigned look, you send your code out for review and wait.

Ten minutes later you get a response… with a single comment.

“Looks good to me, ship it!”

If you don’t recognize this situation or have never been on the sending or receiving end of this, know that this is my nightmare. In situations where there is barely any time put into preparing the code for review or when there is even less time spent reviewing the code, it’s a recipe for disaster. These situations are not uncommon, even at top companies. They don’t always lead to disaster but certainly don’t take advantage of a process that can teach the author and reviewer more than they expected. I certainly didn’t think code reviews would be such a big part of my career when first starting off years ago, yet initially, I struggled with anxiety around them and the desire to defend every line of code changed. While I cannot claim complete victory now, I believe I’ve become more objective, thoughtful and every once in awhile I’ve been told I actually contribute to a meaningful discussion!

My journey has taken me through a few startups in 2010 that lead to two years at Google where I left to help start Convoy over four years ago as a founding engineer. I am lucky enough to have had many great mentors and have had many humbling code reviews that have helped shape the way I think about programming and how I give feedback to others. During the early stages of Convoy, we felt an overwhelming sense of urgency to achieve an MVP which was in contrast to Google where we had a lot more time to work with. However, in both scenarios, the goals were and continue to be the same and the theme of not willing to sacrifice code quality is still very strong.

A tall tale worth telling

At Google, I picked up a few great stories, but when talking about code reviews one story stands out amongst the rest.

If you’re unfamiliar with a concept inside of Google called “readability,” it’s an internal certification that shows the author understands how to write code using Google’s best practices and style guides. All code merged into Google’s codebase must be reviewed by someone with this certification. The process to apply for readability can be very long and oftentimes difficult. This process mandates that the seeker of readability submits a large code change that proves they have sufficient domain knowledge ranging from library use to syntax. The diff is put in a queue that can take up to 9 months before you are assigned a reviewer and then a few months after that until they review your code. The reviewer pours over every line of code you wrote with a fine-toothed comb while highlighting any esoteric nitty-gritty detail they deem needs to be fixed. If they find your code needs too much work and doesn’t match their expectations you may need to find a better example and re-submit. Ouch.

The wait is almost as legendary as the scrutiny that is put into these reviews. To walk away with up to 20 comments is a blessing, more than that and you’re going to have a bad time. However, to finish with just a few comments is quite rare indeed. In fact, there is a leaderboard for the esteemed few engineers that manage this feat, and needless to say, it’s a shortlist.

That’s why my jaw hit the floor as I looked over and saw that my co-worker walked away from his readability review with ZERO comments! …… “H-How! How was that even possible?!” I blurted.

As it turns out, it came down to a lot of hard work with some crafty ingenuity mixed in. I’ll explain later in this post how specifically this was achieved.

What’s the goal of a code review

For my last detour, I think setting expectations on what to get out of a code review is important. Discuss this with your team because they may differ and change how your team functions, but I believe the common goals of a good code review come down to:

1. Improve readability. As the first reviewer of new code, if something isn’t clear to you, this is probably an indication it won’t be clear to others in the future.
2. Knowledge share & personal growth. Use the review as an opportunity to grow and learn something new and as the author, this is one of the best ways to pass learnings onto others.
3. Improve code quality & find bugs. Keeping the quality bar high ends with a good code review. Additionally, there was a 2002 study done for NIST that shows the further away an engineer is from requirements gathering & coding phase the higher the cost is associated with fixing bugs. Though the cost associations are likely different today, I find the principle to still be true, the further away you are from a bug the more expensive it is to fix.

How to review another’s code

Finally, what are some ways to achieve these goals when it’s your turn to review someone else’s code? Below is a mental checklist I put myself through during the lifecycle of a code review. They help me achieve the aforementioned common goals but are certainly not an exhaustive list.

1. Are you feeling rushed? Stop Ask if you can schedule a time to look at the code later when you’re not so distracted and then schedule a time for yourself to do the review.
2. Are you not a domain expert? Speak up If it’s the code you don’t feel comfortable reviewing (unfamiliar codebase, not a language you use often, etc.), let the author know you need extra time and they should probably find a primary reviewer and treat you as a backup.
3. Play with the code Don’t just read the code; run it and prototype some of the things you want to suggest! You might just gain a deeper understanding of the issue and be able to give better feedback to the author.
4. Are you confused? Pair up A pair code review can clear up a lot of misunderstandings and speed up the back and forth quite a bit. Though take care not to encode bad practices because the author convinced you in person. Spend time reviewing outside of the pair review to make your own decisions.
5. Is the diff too large? Break it up Asking the code to be broken into chunks and to separate any unrelated changes can help keep the review scoped and more efficient. A good diff has a single purpose (e.g. a bug fix) and it’s less than 50 lines of code before tests and comments.
6. Is the review ready? Ask for more Working through a product checklist before and not after writing code can lead to architecture changes as well as reduce churn in the product. Make sure tests, experimentation or feature flags, instrumentation, release strategy, etc. are taken care of upfront.
7. Bundle the tests The author’s code should be bundled with the tests to bring additional clarity to the reader, to make reverts easier, to avoid forgetting to write them later, and to discover upfront issues.
8. Avoid unnecessary feedback loops Avoid lengthy back and forth by doing extra work upfront:
9. Read the description Often times authors anticipate confusion, call them out, and readers skip over those critical details.
10. Don’t skim Code that looks mundane is often the key to a change and should be highlighted as such.
11. Do your homework It’s easy to question choices or suggest refactors without thinking through the ramifications as to why the author didn’t do that initially.
12. Ask for an explanation, but come prepared Come with a few theories first before pushing back and asking them to explain.
13. Get someone else involved Bring in a 3rd party perspective if a consensus cannot be reached. Perhaps someone more familiar with the codebase or an engineering manager who is more removed from the smaller details.

If you made it this far and feel a bit overwhelmed, take it one step at a time as the biggest take away is to slow down, dive deep and really understand the impact of the change.

How to prepare your code to be reviewed by others.

By expecting so much of others, when it’s your turn, make sure you help your reviewer as much as possible:

1. Give the proper context take screenshots, write a really descriptive summary of changes, link to issues this code fixes, design docs, or prior art, etc.
2. Review your own code Go through the entire review process yourself first and fix any issues you find.
3. Do not make excuses for yourself Go the extra mile upfront. For example, if you know of a better pattern to use and you’re not using it, don’t leave it to the reviewer to call you out.
4. Make good commit messages Messages like “work in progress” or “fix” don’t help explain the why behind the change. Leaving a history that’s easy to follow will help track down intent and will often be the first evidence looked at when exploring why a change was made.
5. Are you making a dangerous change? Get more help If you feel your code is hitting a particularly dangerous code path, get two reviewers and require both of them to be thorough. Set up instrumentation, write extensive tests, and let others in your org know a change is happening so that you can open up the opportunity for additional feedback.

Don’t be a jerk

Communication is a very important part of both giving and receiving reviews, you could follow all these tips and go about it with a poor attitude and make life difficult for someone trying to submit a code change. At this point, the means don’t justify the end.

1. Default to trust When inquiring about the actions of another, assuming that they had the best intentions.
2. Be polite Say please and thank you when asking for changes, especially difficult ones.
3. Lose the ego For example, instead of saying “Your suggestion doesn’t work” say “This fix won’t work, because…”
4. Be clear Call out what is a request versus a requirement and don’t hold a code review hostage!
5. Don’t use shorthand Often times terse comments or responses can come off poorly and are hard to understand. Use complete sentences and give examples.

Just don’t be a jerk, you’re both on the same team and the person behind the code is more important than the code itself.

In summary

We all have so much to learn from each other. Code reviews really are a good way to become more knowledgeable about your codebase, the languages you use can push you to learn more about the product you’re building, the customer you’re serving, and can help you grow as a person. It really is a treat to find new ways to improve. Not doing due diligence reviewing someone else code as thoroughly as possible you’re denying potential growth for that person as well.

Slow down, dive deep and do your research, treat others with respect and love to learn.

So how did my co-worker at Google manage to get the highly esteemed position of zero comments while under such intense scrutiny? Actually, his idea was simple, but it took a lot of work. After being assigned his reviewer but before they had a chance to review his code, he looked at the recent readability reviews for that person (and several other reviewers), found every pedantic change they ever asked for and made those changes first before they could ask. It took going the extra mile and doing what wasn’t expected of him to become as prepared as he could be but his upfront work paid off itself, good work!

This engineer in question is Ian MacLeod who is another founding engineer of Convoy and a mentor to me. He and I have been hard at work building Convoy along with over a hundred other software engineers. Our mission is to Transport the World with Endless Capacity and Zero Waste and we try to edge closer to that goal one code review at a time.

Come join us!