Effective Code Review
Code review. Some people love it, some people avoid it. As with most engineering processes, code review can be implemented well or poorly. Let's look at how to do it effectively.
What are the goals of code review? The ones usually mentioned are:
- Catching bugs before they ship;
- Improving code base quality;
- Knowledge sharing between engineers.
On a more fundamental level though code review should be aligned with the goals of the engineering organization overall, which - for an agile engineering organization - generally are to ship software that best meets requirements, and has the highest quality, on the shortest possible timeline. Naturally all three of these axes cannot be optimized for simultaneously, hence different organizations will have different priorities - and these priorities should be reflected in code review processes as well.
Going back to the goals of code review, we can restate them as follows. An effective code review process should:
- Reduce occurrence of bugs in the code;
- Improve quality of the code;
- Expedite engineering delivery.
These goals are similar to the previous set but subtly different. The difference is that these goals are all relative. For example, in an agile engineering organization code review is not meant to eliminate bugs completely - the goal of code review is to ship code with fewer bugs than if there had not been a code review process in place.
With these goals in mind, let's take a look at the principles of effective code review.
Whenever I perform code reviews, I keep these principles in mind:
Only make comments that move the needle. There are often many ways of doing something in software, and unless there is a significant benefit of doing it one way vs the other way it is not effective to make people redo work which works fine. Every engineer has their own preferences and changing them takes effort. The effort should be justified in the output as it helps the engineering organization achieve its goals, as discussed above.
Focusing only on real issues in the code being reviewed speeds up both the review process and the development process overall.
Recognize your limitations, and trust others. Especially in larger code bases it is not possible for one person to have complete knowledge of each line of code. This is why larger projects have module owners, people who know and are responsible for a certain part of the overall system.
When performing code review, comment on issues that you have high certainty of, and give the rest of the code the benefit of the doubt. In the event that the code works, you would have potentially learned something, and if it doesn't work then the person who wrote it would have learned something.
This applies more when reviewing code out of your primary area of focus and less when reviewing code by junior engineers.
Timebox. Code review shouldn't take a long time; if it does, either the code is not being written properly or it is not being reviewed properly.
Separate changes should be submitted as separate diffs/patches/pull requests, and reviewed separately. A massive diff that combines a refactoring with a new feature is very hard to review. Engineers should understand this and not make such diffs in the first place, which is a matter of engineering culture. Faced with such a diff I would tend to give it a surface reading and talk to the engineer about splitting up their work in the future.
Refactoring diffs should be easy to review as only the concept behind the refactoring generally needs to be approved - code moves themselves are largely mechanical.
Diffs that pertain to a single feature but are very large often indicate a failure in the review process, namely that they are not looked at early enough when they are still small. The solution is partially to encourage and train engineers to perform early, effective code review and partially to move the engineering organization's culture toward early and more frequent code reviews.
These are the most common code review-related issues I've seen over the years in both commercial and free software projects:
Not following up. Requesting changes and then not following up when the changes are made is probably the worst offense when it comes to code review.
In a corporate environment, lack of a timely follow up also falls into this category.
Not reviewing. The second worst offense is not reviewing the code at all.
Open source projects get some slack here because maintainers are usually volunteers, but even so any maintained project has a "reasonable" code review timeline that should be followed.
In the corporate setting, waiting for code review is a blocker to engineers, just like waiting on requirements would be, and should be treated as such. I usually prioritize reviewing code above writing code (and this works because code review is timeboxed, as detailed above).
Nitpicking. Reviewing and leaving comments like "What you've done works but I would've done it this way" is third on the list. It's annoying, and reduces motivation to contribute or to propose changes to how the code is presently written. I use "nitpicking" in a very broad sense here, because that's how it is perceived. For example, it is very difficult to perform code base-wide refactoring in a project with a nitpicking culture - the first diff to do so would get so many comments that the contributor is likely to give up, provided they even sent a diff in in the first place.
Rejecting changes with no path to fix them. This can be phrased along the lines of "We want this functionality but the way you've done it is not acceptable, and we don't know how to do it in an way that we like". The work that was done is thus in limbo, and time spent on it has been wasted.
If the functionality is indeed wanted, it is often better to accept it as implemented and revise down the road if a better approach is found.
Pointing out obvious non-code review issues. If a pull request failed the test suite and this is clearly indicated in the pull request, there is no need to use the "request changes" option on code review with the comment that the tests failed, as doing so is simply noise. Depending on the project I would either not look at any pull request with failing tests, as this would be a waste of the reviewer's time, or approve/reject the pull request based on its content without taking into account the test suite status, as the tests must be green for the pull request to be mergeable anyway.
Organizational dysfunction spilling into code reviews. An example of this is a policy requiring a single branch, or a pull request, per product change contradicting a best practice of separating refactoring from feature development. An organization following such a policy to the letter paints itself in a corner when it comes to reviewing code because either the diffs are enormous or no refactoring is happening, leading to tech debt never being paid down.
Many engineering organizations do not perform code reviews effectively. By far the easiest way to fix that is from the top down. For example, in a team with no code review culture the lead can start requesting code reviews from other engineers, in the process of doing so guiding them to perform effective reviews as outlined here. It is often very difficult to affect the same change from the bottom of organization.