This post will focus on the value of our teams having a Code Review Checklist. However, just to be sure we have a shared understanding of the benefits of Code Reviews, we’ll take just a short moment to remind ourselves of their impact. The following quote was published over a decade ago and some teams still don’t incorporate Code Reviews into their daily development practices.
“Individual inspections typically catch about 60 percent of defects, which is higher than other techniques except prototyping and high-volume beta testing. These results have been confirmed numerous times at various organizations, including Harris BCSD, National Software Quality Experiment, Software Engineering Institute, Hewlett Packard, and so on (Shull et al 2002).” – Code Complete, Second Edition, published 2004
The fact is, the compiler doesn’t care how clean and well organized our code is. Its job is to produce CPU instructions exposing features to our clients. How we write the source code is purely for other humans to read and understand. It should have the same consistency in structure and organization throughout the entire code base. It should read like a book!
Our typical Software Code Review process inspects the code to ensure it…
- Adheres to the architectural design methodology.
- Meets the intended feature requirements.
- Meets the coding standards and is generally readable and supportable.
All of this could be done by just previewing the code in our source control system (GitHub.com for example).
However, how do we know it…
- Builds without errors or warnings?
- Works according to the intended requirements?
- Performs well and doesn’t throw exceptions?
- Passes all the requisite automated tests?
Our Code Reviews should be comprehensive. They should be documented in our Development Guidelines document and shared with the entire team to facilitate consistency and high quality. The details of our team’s Code Review Checklist will vary and be tailored to each team. However, one example might be:
- Pull down the code being reviewed into Visual Studio (or your development editor of choice) and perform a file by file inspection of the code considering/validating:
- Absolutely no violations of the architectural design principles
- Review the design artifacts (i.e. Activity Diagrams, Call Chain Diagrams, Interaction Diagrams, etc.) therefore ensuring the work satisfies the required core use cases
- Adherence to the Coding Standards and proper naming for readability and supportability of the code
- No obvious poor performance implications or concerns
- No Security violations or concerns
- Build the code and run Code Analysis (or your static analysis tool) to provide feedback on any rule violations
- Execute all Unit/Integration Tests measuring code coverage and providing feedback when coverage is insufficient
- Run the Test Client (when applicable) and evaluate the intended functionality is working
- Merge, close the Pull Request, and delete the branch. This may be left up to each team; however, the benefit of the Code Reviewer officially committing the changes to source control imposes a level of ownership and accountability for their review. It also alleviates any ambiguity that the review is complete and that any/all concerns have been satisfied.
- When applicable, validate the functionality being reviewed in an automated build that has been deployed. This is particularly important when configuration and/or hosting changes have been made.
These are just some of the basics, and each team should embellish this checklist to meet the needs of the team and project. For example, if we have database model changes, we may need to include reviewing the update scripts used for deployment. We would add to this checklist any of our environment or project specific steps to ensure the quality and consistency of all artifacts remain high.