Effective Code Review

Code reviews are an important tool in every engineering organization, with lists of benefits just a quick Google search away. With that said, simply ‘doing’ code reviews is not always enough. It is important to ensure that teams are focused on conducting effective code reviews, and are not just going through the process in order to check a box.

Most of the points I will mention here focus on ensuring that a code review is:

• a good use of time,
• used as a means to improve team members individually

Your team may have slightly different goals, and as such the points that you focus on may differ.

1. Don’t be a linter

Linters are free, fast, and built into (or a quick extension away) in almost any text editor/IDE/CI system you could find to use. Knowing this, one should avoid acting as a linter when performing a code review. Spacing, tabs vs spaces, extra newlines, and variable casing are not something that should be focused on in a code review. Time should be spent looking for potential vulnerabilities and inefficiencies, as well as understanding how this new code (which you are likely going to have to come back to at some point) functions. If you have detailed knowledge of affected code area, a code review is a good time to ensure that new functionality adheres to assumptions made in the codebase, isn’t unnecessarily complex.

In the event that there are serious linting problems in the code, this is a good opportunity to suggest to, or assist the author in getting an automatic linter set up with the project appropriate settings (or even a pre-commit hook), which would fix this problem forever. If you and your team are not using an EditorConfig setup, this may be a good time to add it.

2. Target your feedback to the Author

Code review feedback for a brand new, fresh out of school graduate should differ from that directed at an author who has years of industry experience, and has been at the company for a while. With junior engineers, it is often more important that they are comfortable getting code into production, and confident in their abilities, than it is that they are using a few unnecessary variables, or have a chunk of code which could be pulled into it’s own function. While these types of review comments are perfectly valid, and should be included in a review for a more senior colleague, publicly hammering junior employees with too much information will often have a negative impact on their self-confidence, and lead to hesitation in publishing code. These are things which are better taught one on one, or through pair programming, rather than publicly on a code review.

Note that I am not advocating that you knowingly allow unmaintainable or incorrect code to be merged. I am talking specifically about situations where the existing code is “good enough”, but may not be “perfect”. In effect, you do not want to focus too much on a perfect solution that a junior engineer feels that they are never able to release code, which will likely have a detrimental effect on their productivity.

3. Focus on the next time

Building on #2, many comments are informative, and help to improve an engineer moving forward. It is often the situation where the current solution will work perfectly fine, however these exists a more standard way of accomplishing the same task. Making these comments can often come down to phrasing; instead of saying it needs to be changed, or even stating that it could be changed (which will often be interpreted as “it needs to be changed”) it is good to state the other way, make it clear that they way they accomplished the task is also valid, however in the future it should be accomplished in the standard way. This has the advantage of feeling like a win for the engineer, while enacting good habits.

Some people worry that this will leave ‘bad’ code hanging around the code base. If your team has a habit of cleaning up code as they come across it, chances are that the ‘bad’ code won’t survive very long anyways.

4. Continuously Increasing Scrutiny

As an engineer evolves, the scrutiny give to their code should evolve as well. Clearly a code review for a senior engineer written as if it was targeting a junior engineer will not drive them personally improve. For instance, letting small issues and style pass for a junior engineer will give them confidence, however it comes with the expectation that it will improve in the future. Letting these things slide with a senior engineer does not serve that purpose, and instead the lack of scrutiny allows them to glide through their tasks without putting in an effort to improve.

5. Do not block merges unnecessarily

I think the title speaks for itself on this one. Github (among others) have the ability to ‘Request Changes’, and block the ability to merge a PR. Please, please do not use these unless there is a large enough problem. What someone believes is ‘large enough’ is mostly an individual decision, however I use the metric of “could making the change I requested change the outcome of executing this code”. For example, making a stylistic change, or a small optimization will not change the result, however making an optimization which will prevent a request from timing out does. If the answer to this question is “No”, blocking a merge is most likely unnecessary.

6. If you won’t review, Don’t review

This is probably the most useful piece of advice for junior engineers who are starting to do code review: If you are not going to actually review the code, do not do a code review. There is nothing more disappointing than a 1000 line PR modifying multiple services getting a “lgtm :+1:” and an approval after 5 minutes, with no substantial comments.

Giving an effective code review requires time to commit to the task at hand. Without budgeting enough time to fully read the code, and understand what the goal is, you will rarely end up with a code review that satisfies your teams' improvement goals. At the end of an effective code review a reviewer should be able to answer basic questions (if asked) about the subject matter. If those questions can’t be answered, then not only was the code review likely non-useful, but likely it was a disservice to the entire team by giving a false sense of security.

Special thanks to @nosa_manuel and @joshdover for the early review and input on this post.

Hopefully this works out for you, or was at least helpful. If you have any questions, don’t hesitate to shoot me an email, or follow me on twitter @nrmitchi.