In this blog post, Erik Dietrich goes over creating a code review checklist. If you were to Google “Code review checklist”, the author lists results that may show up:
- Does every method have an XML comment?
- Do classes have a copyright header?
- Do fields, methods, and types follow our standard naming convention?
- Do methods have too many parameters?
- Are you checking validity of method parameters?
- Does the code have “magic” values instead of named constants?
He then goes on to list two problems with going through a sometimes lengthy checklist:
- You can’t keep 100+ items in your head as you look at every method or clause in a code base, so you’re going to have to read the code over and over, looking for different things.
- None of the checks I listed above actually require human intervention. They can all be handled via static analysis.
His suggestion for stream-lining the process of going through a big checklist is to automate the easy stuff. “Get static analysis tools that developers can install in their IDEs and run prior to delivering code, which will flag violations as errors or warnings. Get static analysis tools that run on the build machine and fail the build for violations.”
Code Review for the Important Stuff
The author lists an example checklist for a code author:
- Does my code compile without errors and run without exceptions in happy path conditions?
- Have I checked this code to see if it triggers compiler or static analysis warnings?
- Have I covered this code with appropriate tests, and are those tests currently green?
- Have I run our performance/load/smoke tests to make sure nothing I’ve introduced is a performance killer?
- Have I run our suite of security tests/checks to make sure I’m not opening vulnerabilities?
The author lists an example checklist for a code reviewer
- Does this code read like prose?
- Do the methods do what the name of the method claims that they’ll do? Same for classes?
- Can I get an understanding of the desired behavior just by doing quick scans through unit and acceptance tests?
- Does the understanding of the desired behavior match the requirements/stories for this work?
- Is this code introducing any new dependencies between classes/components/modules and, if so, is it necessary to do that?
- Is this code idiomatic, taking full advantage of the language, frameworks, and tools that we use?
- Is anything here a re-implementation of existing functionality the developer may not be aware of?
I chose this resource because it had very useful information on code review, which ties into QA. I feel the content of the article is very informational and useful to my future career. It is a good starting point for making sure you have thoroughly checked the quality of your code and automated tests are only going to increase in use, so it makes sense to automate what you can.