A practical guide for conducting efficient code reviews - Heart Internet Blog - Focusing on all aspects of the web

If you’re lucky, you’ll find yourself working at a company you like, with team members you appreciate. It’s a shame that’s being ‘lucky’ because it should be the default. In any case, you’re lucky and you enjoy your work — at least most of the time — and the people you work with.

I’m one of those people, I consider myself lucky to work at my current place. Our product team — developers, designers, and product managers alike — enjoy the work we do, the people that help us do it, and the challenges we face. I like the work I do mostly because I value the team of developers we have and the culture we developed among us. We care about each other, we care about the product, and above all, we care about our craft. We seek learning: we have weekly sessions to watch and discuss technical talks, we have a fortnightly ‘investment day’ (where we can work on anything that benefits us, the company, or the community), and regularly — daily, actually — we review each other’s code.

Virtually nothing gets into our production environment without review. Even when we pair up to solve an issue, there’s usually a third person involved in the PR (pull request). Frequent peer reviews have been around since I’ve started, and we’ve all learnt a lot with them and about them. It’s one of our core beliefs and, although their scope, length, and depthness change from time to time, basically every line of code in our master branch got at least two pairs of eyes on it. Besides scoping, developing and testing features out, reviewing each other’s work is an essential part of our daily work.

Given these past years of daily reviews, interactions in person and via GitHub’s pull requests, and frequent discussions about the merits of different approaches, I grew to believe that, either by design or sheer luck, we got a few things right about this process. These seem so natural to me now that I had to take some time to look back and see how far we’ve come and how much we have improved. I now feel confident in identifying a few points that both reviewers and reviewees try to have in mind when requesting code reviews and when giving feedback to each other.

Reviewers

The biggest part of carrying out a code review falls into the hands of, without much surprise, the reviewer! You were asked for feedback (or you volunteered), so now you have a good part of the fate of the review in your hands. Whatever the pull request throws at you, you have the power to handle it in the best way possible. You can be kind, sympathetic, and informative.

I would argue that the two main goals of peer reviews are code quality assurance and knowledge spreading. One of the great advantages of code reviews are bugs, or rather, their reduction. Studies focusing on quantifying the bug-catching abilities of peer reviews have pointed out that even lightweight review processes are very effective at catching defects before they affect our users. Secondly, it allows us to share knowledge. We can learn or teach something about the business, about how our applications or infrastructures work, or even about the language we write it in. Peer reviews break project sylos, when you pick up a review of a feature you are not working on at the moment. They help you level up, as the most intimidating and time consuming code reviews are the ones that will probably teach you the most — don’t shy away from them. Now that I (hopefully) convinced you about their value, let’s look at them in more detail.

Understand the context first

First things first, don’t jump straight into the code, not just yet! Read the pull request description. Then read it again. Try to explain it in your own words. If you can do that, now you are ready to look at the code. If there are comments along the PR, read them carefully. Having a clear idea of the context around the issue or feature at hand will help you to evaluate the decisions taken by the author of the code you are assessing right now. In a perfect world, all code would be polished and beautiful and resembling an amazing monospaced haiku. However, in the real world, sometimes we write code we are not proud of because conditions forced our hand. We should all be more understandable of those situations.

Ask, don’t tell

Unlike a familiar idea in object-oriented code, suggestions you might have are usually better phrased as questions. Instead of telling people, “you should use service XYZ instead here”, why not ask “could we use service XYZ here instead?”. At first glance, the change is imperceptible. However, unconsciously we react differently to these two. Nobody likes to feel bossed around. Also, “we” is friendlier and more collaborative than “you” — we’re in this together. As soon as it gets into master, your code becomes our code; we all end up maintaining each other’s work.

Asking goes even beyond this. It allows you to approach the pull request from a point of curiosity and interest instead of challenge. Don’t be afraid to ask about things that you don’t understand! Ideally, the code you read should be graspable as quickly as possible. If after reading it a few times you’re still confused, you might have stumbled onto something; the code you are reading might not be as clear as it could be. If it puzzled you, it will probably puzzle the next developer that lays eyes on it. Work with the author to clarify and simplify it, pair on it if possible — I’ve found real time collaboration a much more streamlined way of doing a peer review.

Finally, sometimes you’ll find yourself questioning the author of the code about something because you have a concrete concern about their code. Tell them about that concern. “Why did you use this service?” puts the author of the code in a defensive position, but if you add your concern (“Could you tell me more about why did you use this service? I’m concerned about its performance in this scenario.”) will allow reviewees to put themselves in your shoes when reading your comments.

Improvements have a threshold

This is somewhat of a recent idea we got from Alexandra Hill quoting Michael Lynch. As reviewers, we look at every pull request through the same quality lens. We aspire every line of code to be the best version of itself. If we think of code having a letter grade, from A to F, what do we do if the submitted code has a low grade, D for example? In this situation, it can be frustrating having back and forths to try to get it to A. If it comes from a low grade from the get-go, it’s a bit unrealistic to expect it to excel simply with a code review. Instead, work with the author to get it to C or a B-. It won’t be perfect, but it will be good enough.

I realise this practice is not for every team, maybe you do need to have perfect code on your job (I would bet NASA doesn’t cut any slack to developers)! However, we try to strike a balance between code quality, performance, and developer happiness. It’s frustrating to submit a pull request over and over again, just to be met with more and more requests for changes. It’s equally frustrating to review a feature multiple times that has issues in our opinion but not according to the author. It is exhausting for both parties to go over a never-ending cycle of request changes and having to justify current code. It can become a source for resentment rather quickly.

Differentiate blockers from non-blockers

Undoubtedly, while doing a code review, you’ll find some things you don’t agree with. Some things are personal preference, other things are just wrong or even dangerous. It’s quite important to differentiate a design pattern we’re not using correctly from different opinions about code indentation. The former could lead to technical debt (or even bugs), the latter is a simple preference that is hard to settle (and doesn’t really matter in the long run).

A matrix showing conflict vs reward for certain scenarios
Worthwhileness vs Conflict Potential, a matrix by Alexandra Hill

Automate as much as possible, especially the simple issues that can create long discussions but that are not that important. Tools like HoundCI for automated linting or Prettier for standardised code-formatting are ways to reduce the friction between people and focus reviewer and reviewee on the essence of the code. Agreeing on a coding style and setting automated tools to promote them will free up people’s time to have better discussions.

Reviewees

I find that most guides, manuals and tips about code reviews focus too much (or almost exclusively) on the people giving the feedback, while forgetting about the people receiving it.

For this matter, I draw as much as I can from Stoicism — a life philosophy from Roman times, where people were already worried about how to react to external factors. One of its core thoughts is that you only have control over your own reactions, so that’s where you should put your effort. Getting sad or angry over a harsh, unjust review you just received is, although a natural reaction, a waste of time in the eyes of a Stoic sage. The feedback you get doesn’t have an inherent good or bad value, it is you who gives it the value you want. I don’t know about you, but I find this view empowering, so I try as hard as I can to see the bright side on every comment I get.

If you don’t find the feedback you got to be as positive as you were expecting, try to put yourself in the reviewer’s shoes — maybe they had a terrible day and something pushed them over the edge. You can face this as an opportunity to practice patience and genuine care. You can pluck the good and objective out of the comments you got and make the best out of it. Maybe it can help you improve your communication and argumentation skills. Yes, the reviewer was not kind at all, and all of the feedback came from a rushed place, but you can’t change that anymore. What you can change is how you answer to it; you can replace that automatic, snappy System 1 response with a kind, thoughtful System 2 discourse (you can read more about it on Thinking, Fast and Slow).

Before submitting anything to review though, a helpful practice we encourage at my company is to do a self code assessment before putting your work out there. Slowly scroll through the pull request you are about to open and take some time to look at it with a fresh pair of eyes. Put yourself in the shoes of someone who’s new to this code, pretty much the position the reviewer will be. Try to anticipate some questions that will come up and, if possible, sprinkle some comments where you think you faced a trade-off and you want to clarify the route you took. We’ve found this to reduce rushed mistakes and premature requests for changes, allowing reviewees to be more mindful of what they are submitting.

Always assume the best intentions

If the reviewer’s feedback seems rather dry or even unkind, remind yourself of the amount of context personal interactions come with that you might be missing because you’re just reading their words. Some people supplement their messages with enough emojis to amend this missing information, but some people don’t. This will require you to infer what did the reviewer mean here and there. I advise you to be kind on your assumptions and to frequently assume the reviewer had the best of intentions.

If you find it hard to do, try to remember that your peer took time out of their day to help you, reading the code you wrote and providing helpful tips. They are probably as busy as you are, and yet, they put aside enough time out of their day to help you move your task along.

If you find yourself getting annoyed by the feedback as you read along, just stop, close your eyes, empty your mind and go back to the start of the sentence. Start again with a calmer, gentler frame of mind. Take a few moments to ponder over what you read, don’t answer right away. Take the time to see the point of whoever wrote what you just read. Make sure you understand it and ask questions if you don’t. Thank them for the suggestions, for the time they spent going over your code. Keep a positive lens over whatever you’re reading.

Don’t get defensive

Don’t ever let feedback get to you. If you received negative feedback, remember that the feedback is about what you wrote, not about you. Every great developer writes terrible code from time to time. Don’t let that odd less-than-ideal code make you doubt your abilities, don’t let it feed any lingering imposter syndrome.

Since the feedback is simply on the code that you wrote, try to look at it as critically as the reviewer did. For a minute, try to forget that you are the author and imagine what you would think if someone else in your team had written it and asked you to take a look. Making an effort to be impartial and evaluate the code itself will help you to ignore any negativity from the feedback you received and take in only the helpful stuff.

Give as much context as possible to the reviewer

Have you ever found yourself angry over something you’ve read only to realise later that, in fact, you were actually confused about it? Confusing pull requests can lead the reviewer to write harsher words. Provide as much context as possible: links to tickets about the spec of the feature you’ve built, descriptions of how to reproduce the bug you’re fixing, even screenshots and videos of interactions.

As previously suggested, we’ve found that it really helps the reviews if you leave comments on sections that could lead to questions. Just skim through the code you’re about to submit for review and look at it with fresh eyes. Do you see a tricky trade-off that could spark controversy? Be proactive and leave a comment there and then, explaining the decision you took. Showing that you understand the pros and cons of your approach will preemptively answer questions from the reviewers and show them you were mindful of the issue.

Small changes make everyone happy

This one will help you particularly if your peer reviews are volunteer-based, like ours. We have a Slack channel simply for code reviews, where we post the pull request link, its title, and the line difference (+XX lines added, -YY removed). Smaller diffs (or clear explanations for bigger diffs) dramatically increase the chances a developer with a few minutes to spare will pick up your request and review your code.

Keep the changes on the PR small and focused. Keep the concerns of the pull request to a minimum, splitting it up into multiple ones if the changes you made have several responsibilities. This will represent a smaller mental load for reviewers, allowing them to focus on a single thing and do the best possible job at it. Your ability to find defects in the code goes down as the number of lines you have to review goes up, according to a Cisco code review study.

A graph showing the more lines of code you have to review, the less defects you find
The more lines of code you have to review, the less defects you find

Having a smaller diff will also help with readability. As developers, we actually read more code than we write, so readability is an important concern. Keeping a small and sensible diff, with just the right amount of lines changes will help the reviewer browse through your changes and understand everything better. This consideration even influences my coding style; for example, I’m a fan of trailing commas: if a user appends an element at the end of the array, they don’t need to add a comma to the previous line, it would be already there.

Summary

Finally, I would like to add a big asterisk to everything I’ve written so far and add the following: this is what worked for our team, but your mileage might vary. Different types of people create different teams, with different cultures and wildly different practices. What suits our team might not suit yours, but we didn’t get to these ideas in one go. It took years of working together, weekly discussion and iteration through retros and fine tuning. I encourage you to share your own thoughts, insights and practices because it was thanks to posts like this one that we developed some of our ideas. Hit me up on Twitter if you would like to share them with me.

Further Reading

Subscribe to our monthly Heart Internet newsletter, filled with the latest articles about web design, development, building your business, and exclusive offers.

Subscribe now!

Comments

Please remember that all comments are moderated and any links you paste in your comment will remain as plain text. If your comment looks like spam it will be deleted. We're looking forward to answering your questions and hearing your comments and opinions!

Leave a reply

Comments are closed.

Drop us a line 0330 660 0255 or email sales@heartinternet.uk