How to review code
I’ve reviewed over 3000 code changes so far in my career, and gone through about same amount of reviews as change author. When reviewing code, here is the golden rule I follow:
Any code change that’s a strict improvement should be approved.
Most incoming code reviews are both improvements in some aspects and make things worse in other aspects.
Understand the change
It’s tempting to glance over a change and only comment on small technical details or style issues, but this will eventually lead to worse code. In order to give meaningful feedback, understand the change to the extent where you could have authored it yourself.
Blocking issues to look out for
New code is unclear. Ask: “I do not understand this”. Insist that it’s clarified in the final submitted code rather than in a review comment.
New code is missing tests. Insist.
New code does not fit into surrounding code in terms of style or common libraries used. Insist. Link to a canonical style guide whenever you can.
Pre-existing tests were changed willy nilly until they run green. This often takes the form of only a few lines added to every existing test case. These drive-by fixes accumulate easily over the course of some changes, until the core idea behind the test cases is obstructed. Insist on refactoring the tests.
New code introduces duplication with existing code. Ask to deduplicate. (This is particularly often seen in tests.)
The change is increasing code complexity.
- Is there a better solution you can suggest?
- Does the change’s benefit outweigh the additional complexity?
The change introduces dependencies in places where they don’t belong.
Non-blocking issues to look out for
It’s fine to ask for additional changes that are not strictly required, but always make it clear that they are optional.
“I would have done it differently.” is a gut reaction almost all reviewers have sometimes. But it’s a shortcut heuristic the mind takes which usually has a more solid technical background.
Learn the specific technical background to your gut reactions, so you can give concrete advice.If you can’t find a specific technical problem, consider asking why it was done in that particular way. There are often be good reasons which only need to be clarified a bit in the code.
Pre-existing issues. Point out if pre-existing issues can be fixed in the same change, but explicitly mark it as optional. Even when not done immediately, this discussion helps to form a common understanding which pre-existing patterns in the code should be avoided and kept.
Taking technical debt: A game of trust
Sometimes you will want to approve newly introduced issues and postpone their fixes until later (Technical debt). Insist that the author will fix these issues after the submission.
In these cases, code reviews are a game of trust. A strategy that works for me is: Assume the best from people you don’t know yet, but don’t let them pass a second time if they didn’t pay off the technical debt.
Good ways to track technical debt issues are (1) a good memory for
names, (2) a shared bug tracker and (3)
TODO markers in the code
(for smaller things).
Behaviours to avoid as a reviewer
Do not sit on a code review. Even if you can’t do the review, explain immediately why you can’t do it (yet) and redirect to someone better suited if possible.
Never insist that reviewees fix pre-existing issues. Being code reviewer is a position of power, but it’s not a free pass to blackmail others into doing your work.
Don’t block legitimate changes for corporate politics reasons. This should go without saying. Do not use code reviews to unjustly influence project priorities.
I hope this collection of code reviewing tips will help someone having more constructive code reviews. Remember:
The reviewer’s job is not to stand in the way, but to enable the code submitter to do their best work.
Feedback is always appreciated. :)