What to look for during Code Review

Read time: 6 mins

“What do you look for in a code review?”

A new joiner in the team asked me this question the other day.

Although I’ve done hundreds of code reviews at this point in my career, I was momentarily stumped. For software engineers, code review is a regular part of the job, and it becomes muscle memory for many of us.

I gave him a generic answer from the top of my head: “check the code is correct…”, “…make sure there are no obvious bugs…”, “…make sure there are no duplications…”.

Although he looked somewhat satisfied with my answer, I wasn’t. It felt wishy-washy and I wanted something concrete.

So what do you look for?

I started talking to other engineers about what they look for and compiled a list of what people look for. In general, it fell into three groups:

1. Code correctness: the code does as it’s intended, and doesn’t introduce any bugs
2. Sets the team up for success: the code is readable, has the right level of abstractions, and has good interfaces. It has comments and documentations for others to understand. It can be reused for future projects. Oh, and it should be tested.
3. Conforms to best practices: it conforms to software best practices, e.g. no code duplications, single responsibilities. It should also follow best practices of the technologies/framework it’s working with.

Making sure the code does what it says

Of the three groups, correctness is the most important – you want to make sure the code actually does its job!

If the code is meant to add a link that redirects to another page, you’d make sure the code does just that. If it doesn’t, or it does something different from what you expect, you’d want to flag this up with the author and ask them what’s up.

This is the most important thing to look for. Personally, I also find this to be the easiest of the three. It should be straightforward to map the pull request title and description to the functionalities.

Talking to engineers, I think they share my sentiment. What we agreed to be more difficult, is thinking bigger picture, which brings me to the next thing to look for:

The code sets the team up for success

Code isn’t for individuals – it’s for the whole team. As a reviewer, you should make sure the code being shipped sets the team up for success. Generally, you would look for the following:

• Testing – the code should test the core functionalities as well as any edge cases that might bite the team later on.
• Readable – the code should be easy to read by the next engineer. This can mean that cryptic code to more readable functions, variable names are aptly named. For some teams that have a consensus about what’s more readable (e.g. for-loops over iterator functions like forEach/map), it can also be called out in the code review.
• Comments – there should be comments on the code where the ‘why’ and ‘how’ might not be immediately obvious to the reader. For example, if you call a function with a particular value, you might want the author to elaborate why; if the author coded a feature with some cryptic 20-steps functions, you might want them to add a sentence or two to elaborate how to give the reader some context.
• Documentations – for functions and methods, there should be documentations on its behaviours and quirks. It should explain its interface and how the team can use them in the future, i.e. what do the arguments mean, how do you call upon this in the future.
• Reusable for future projects – the code should be general enough that it can be reused for future projects. This is especially important for the projects on the horizon – if we know a piece of functionality will be reused for a project that’s coming up in a couple of months, we want to make sure the code is general enough to meet these use cases.

Most of these are straightforward, and there are plenty of resources on how to do these well (Clean Code and Philosophy of Software Engineering comes to mind). There are also plenty of tools that can automatically enforce rules such as test coverage and code rules that will help the team further.

The part I find the most difficult (and one I’m trying to hone my skills on) is to look at the big picture whilst reviewing code, i.e. making sure the code is reusable for future projects. It’s difficult to digest code and think about which future project a snippet of code will help. It’s impractical (and probably not a good idea) to make every piece of code generic – sometimes you just need to ship code quickly for a project.

The code conforms to best practices

The code being shipped should conform to fundamental software best practices, which are applicable to any software regardless of languages and framework. It should also conform to best practices of the technologies it’s working with.

By fundamental software best practices, the code should adhere to principles like SOLID. It should apply appropriate design patterns for solving problems. It should avoid anti-patterns like code duplication, singletons. There are a lot to cover here, and takes a lifetime to master. I’ve included some resources at the end of this post for further reading.

The code should adhere to best practices of the technologies it’s working with. For example, if reviewing JavaScript code with React, you’d make sure the code conforms to JS best practices like avoiding the use of var, always use === (of course, a lot of these can be done automatically with a linter). You’d also want to make sure it conforms to React best practices, like appropriate use of the rendering lifecycles.

Making sure the code you’re reviewing adheres to the technologies’ best practices requires in-depth knowledge of the pieces it’s working with. You’d need to make sure you have a good mental model of the technologies the team is working with (you can also pick up a lot of knowledge by seeing how other subject experts review the codebase). In general, for a new codebase, I stick to reviewing fundamental software best practices as I ease myself in.

Things that should be automated (and shouldn’t be discussed in code reviews)

I’ve covered the meaty topics that I think every reviewer should look for. There are some topics that I believe a team should automate out of the code review because they can distract the reviewer from the important topics like correctness and best practices (also they are generally an unnecessary source of friction):

• Code styling – use a code formatter like prettier that formats the code
• Spelling mistakes – use a plugin that checks the comments/documentations
• Test coverage (percentage) – use tools like jest or Istanbul that fails the build if it falls below a threshold

Final thoughts

Code review is a difficult exercise, but an extremely valuable one for making sure your team ships code that sets the team up for success. It is a great exercise for exchanging knowledge between the teams. As a reviewer, you will learn a lot about approaches to problems and gives you an opportunity to teach others what you know.

There is a lot to look for in a code review, and there are only so many hours in the day to review code, you need to prioritise what to look out for during the review. I hope that following the points discussed in this post (i.e. code correctness, setting the team up for success, and conforming to best practices) would improve your review experience and your team’s code quality.

Finally, I recommend investing time in introducing code linting and formatting rules for recurring discussions during code reviews. This is so that the code reviews actually spot important problems like code abstractions and reusability rather than where the curly braces should be. It would also improve the coding experiences too!

Further reading