So you've just agreed to be a code reviewer for another developers work. What should I check is available before the review can go ahead.
- The problem specification
- Any supporting documentation or knowledge
- Test reports
- Clearly outlined key tests
- Static analysis reports
- Source code or artefact ready for review is committed in source control
The author should provide these clearly to the reviewer. Note they do not all apply to all situations and all artefacts, so a pragmatic approach needs to be taken. It is important not to waste the reviewers' time by making them trawl through large amounts of information. The author must make it easy for the reviewer to provide feedback quickly. This might mean supplying documents or web pages or a short whiteboard session between the author and the reviewers to set the context for the review.
The Problem Specification
Why does this artefact/change exist? What problem is it supposed to solve? This is the primary question we should answer. Obviously we are looking for other things like code readability, code quality etc but does it actually do what it's supposed to do?
Supporting knowledge or documentation
What's the context of this artefact that is up for review. The quickest way to transfer this to the reviewer maybe via a quick face to face session.
Does this change or artefact break existing functionality? Test reports of regression should prove that the changes do not impact legacy.
Take a look at the tests that are accompanying the artefact/change. Do they conform to industry standard like Given When Then? Do the tests reflect the language of the problem? Are they correctly coupled? Are they Good tests?
There are some fantastic tools out there like PMD, findbugs and Sonar. Is your project using any of them? Has the artefact for review passed all static analyses checks? For any failures, are there good, objective reasons why we are violating the rule?