Articles about code reviews tend to be a bit hand wavy when it comes to telling you how they’re done. Virtues get extolled readily, but step-by-step guidelines are harder to come by.
I myself have had to piece together my methodology through trial-and-error. This is what I’ve settled on for now:
- What problem does this code solve?
- How did the author come to decide that this was the best approach? Verify all assumptions made at each step of the decision-making process.
- Is there a better way to solve this problem? Consider non-code approaches as well.
- Does this code solve a fundamental problem or is it just a temporary stop-gap measure?
- If it is just a stop-gap, does the author have an action plan to eventually do away with it?
- Does this change affect any other stakeholders? This includes end users and teammates.
- Can we minimise the effects?
- What concrete steps have or will be taken to minimise these effects?
- Does the code work? Goes without saying…
- Are there any missing edge cases?
- Are there any potential security holes?
- Can we improve code clarity?
- Can we simplify the code?
- Can we remove any code?
- If the original author gets hit by a bus, are YOU — the reviewer — going to be able to take over?
- Do we need to do anything before and/or after merging this code? This usually applies to changes affecting CI servers and your teammates’ development environments.
- Can we deploy this code with zero downtime?
- Do we need to do anything before and/or after deploying this code? Get the author to write a checklist.
I also make it a point to stop myself from commenting on:
- Formatting and coding conventions. Basically anything that can be detected by a linter.
- Tests. Because after all the above, I’m usually out of willpower to give the tests anything more than a cursory glance.
Credit where credit is due: This is more or less how we reviewed code as Ruby programmers at Aiming. And the teammate that drove the point home for me was @ttanimichi.