I wrote recently about checklists for peer code reviews
. We talked about the benefits of peer code reviews and some tips for making them a natural part of your contests. Today, we'll follow up with the checklist I use as a baseline. Note that this checklist will vary a bit based on the technology stack and your team's specific needs, but here's what I use. First, there are some things that I specifically do not put on a code review checklist:
- Anything that will be caught by static code analyzers. This includes things like PEP-8/flake-8 compliance for Python, or memory leak detection in C++ or similar.
- Confirming it builds and passes automated tests. This should be handled by your build system. At most, you can confirm that it ran and these steps passed.
- Code style (sometimes). If there are code style tools in place, then this doesn't need to be reviewed. If this is a matter of conformance rather than a tool formatting the code, then it should be included.
These are the things that are on the checklist:
- Code style (usually). Include this if your code is not formatted through some tool. This covers things like brace location, line length, and method and variable naming.
- Are the "whys" documented? Comments are frequently redundant, but comments about why the developer did something unusual should be there. This is things like, "The first result from this third-party system is always null due to a bug on their end." This also applies to complex functions or algorithms.
- Appropriate defensiveness around inputs. Confirm that input from the end user is scrubbed and encoded. Confirm that third-party utilities are all surrounded with appropriate try-catch blocks and error handling.
- Code factors allow for testability. This varies a bit by language, but covers things like making sure interfaces can be mocked,and that test frameworks can exercise methods. For example, PyUnit doesn't allow testing of the constructor directly, so it's best to keep the constructor slim and put any logic in a method that the constructor calls, which also renders that method testable.
- Tests exist and are comprehensive. This one applies only in situations where the developers are writing tests, which I hope is a common situation! This checks for unit and/or integration tests that go beyond "assert true." How far they go specifically depends on the team, and this is often measured by proxy through checking the coverage level (if it goes down, there may be a problem here).
- The code externalizes configuration. Look for things that are likely to change—number of items on a page in a paginated list, or the options in a dropdown list—and ensure they're modifiable without changing the code. This also includes confirming that they are pulled from your standard location, whether that's a database or a file on a file system, or wherever.
- No commented code. If code is commented out, it should be deleted. The magic of source control can bring it back if we decide we need it after all.
- TODOs. Some code will have todos; the job of the code review is to check that they are acceptable. For example, a "TODO: implement security restrictions" might be okay as long as it's fixed before shipping, but it might not be okay if this release is going out the door tomorrow.
- Loops. Check loops for length and appropriate exit criteria. Also check for speed; loops that can have many objects may be too slow. These things fall into the category of frequent mistakes for many developers.
- Existence checks. Before using objects, check that they exist. This will help put error handling close to the source of the problem. This is particularly true for anything obtained from a location outside the system (e.g., the response to a call to another system, or a file read off the file system).
- Duplicate code checks. Look for code doing similar things; this is an opportunity for refactoring and/or removing duplication. This comes up a lot when a developer is working in an unfamiliar area of the system, or when the system is large and has many utility classes and methods.
What else is on your code review list?