Code Reviews - the rules
The rule is: no code gets checked in without a review.
It’s not always easy to get a reviewer to sign off a changelist. Does the code build? In all configurations and on all platforms? Do the tests pass? Are all new code paths covered? Does the commit message describe the change? Does the formatting match the style guide? Does the code match its surroundings? How about documentation, compiler warnings, license requirements?
Is the change really necessary? Could it have been realised more simply?
Certainly the reviewer’s task is easier if the task has been paired on. Small and self-contained changelists are more straightforward. Removing code, too, should be less contentious.
Depending on infrastructure, some checklist items can be automated. Ideally the changelist has already been though CI, for example, ticking the builds-cleanly and passes-its-tests boxes.
So far, so what? (So obvious!)
Here’s another rule, and one I’ve not seen written down before.
When I review code I might well consider how I would have made the change. That doesn’t mean I’ll insist the submitter does things my way. In the absence of automated formatters there will be more than one acceptable way to lay out the code. Sometimes there’s little reason to prefer an explicit loop over an algorithm + lambda combination, or vice-versa. Short names work for me but not for everyone. It’s hard to argue against test coverage, but is more always better?
In such cases I won’t try to impose my own style on the changelist. Instead, the question becomes: does the code match the standards we, as a team, have set? Or, do these changes merit a place in our codebase?
It’s a simple principle but not an obvious one. It helps me review fairly and also to learn from others on my team.
There is more than one way to do it!
Articles