Code Review Checklist

What follows is the results of a brainstorming session on items that should be in a code review checklist.  As you can see, it needs refining and grouping.  Please feel free to add comments with any items you think should be on it, with any organizational approaches, or any criticism.  Right now, I want to focus on inclusive instead of exclusive, so please don’t recommend removing things:  that willl happen later.

  • Does this code Swallow any exceptions (Bad)
  • Does this code Create any new unnecessary dependencies (Bad)
  • Does this code Introduce any performance bottlenecks (Bad)
  • Is this code threadsafe? (Good)
  • If this code is used only in a single thread, does is use any synchronization?(Bad)
  • Are data transfer objects serializable(Good)
  • Have a unit test(good)
  • Have a functional test(Good)
  • was the unit test run at 100%(Good)
  • Was the functional test run, were there any additional test failures(Bad)
  • Does this code change a public API. (Bad) If so is the change backwards compatable (Good)
  • Does this code have any functions that are more than 30 lines. (Bad)
  • Does this code have any magic numbers or string literals (Bad)
  • Does this code use appropriate internationalization mechanisms for all text visible to the end user (Good)
  • Does this checkin remove Lava code?
  • Is this code platform specific? Should it be?
  • Is this code intended to run on the server, the client, or the managed platform.
  • Does this code reproduce functionality that is done elsewhere in the code base.
  • Does this code use an eternal library with an incompatble library.
  • If this code uses a scripting language, does it hide errors in type safety?
  • Is this code reusable?
  • Does this code go against the coding style of the rest of the project?
  • Has this code been reviewed?
  • Does this code implement the design specified?
  • Has any user interface been reviewed by UX?
  • Has any Database interaction been reviewed by a DBA?
  • Does this code introduce any network roundtrips?
  • Is the message size for any network communication larger than an ethernet packet frame.
  • Can this code gracefully handle a network failure?
  • Does this code use any deliberate casting?
  • Does this code use aany APIs that will not be available to it at runtime?
  • Is this code understandable by someone that is not on the project?
  • Do all files have appropriate Copyright and license headers?
  • Do all public APIs have appropriate Javadoc/Doxygen/perldoc information?
  • Does this code introduce any unnecessary complexity?
  • Does this code work based on undocumented assumptions?
  • Have you made any changes in the code since the last time you ran through the unit and functional tests?
  • Have you stepped through the code in a debugger?
  • Are all reads and writes performed completely, or is there the possibility of missing information?
  • Are any created classes usable when just their contructors have been called, or do they require additional property sets afterwards?
  • Are all resources released when they are no longer needed?
  • Are fields that cannot be change tagged final/const?
  • If this class is going to be called via the bean api , are the appropriate fields exposed via getters and setters?
  • Do property setters handle null? Is null a valid option for them?
  • Must any code in this checkin live inside a transaction boundary?
  • Does any of this code directly manipulate a resource that is supposed to be encapsulated inside some other class or abstraction?
  • Does this code handle all possible exceptions that can be triggered by the code it calls into?
  • Does the code follow the project’s coding standard?
  • Have all files been run through a code formatter set to project specifications?

Contributions from Victor  Erminpour

  • Have we run static analysis tools on this code?
  • Does this code introduce any unintended side effects?
  • Does this code exist somewhere else?
  • Is this code generic and maintainable (i.e., if someone changes a class member, will my function still work?) .
  • What’s the performance impact of the code?
  • Can it be optimized?
  • Is the code secure?
  • Does it introduce and buffer/heap exploits?

5 thoughts on “Code Review Checklist

  1. Greg, Just took a quick read through your article. Very cool. I will definitely keep those guidelines in mind.

    You say things like :nothing that can be automated. Spot on. I was going for “did you run the automation” as a checklist item.

    I see this more like how a flight crew checklist works: if this light goes on, run through this check list. So while the complete list will be exhaustive, the list that someone actually uses will be closer to the 7+-2 rule. I’d see something like: if the code does gui work, include the gui checklist, and so on.

Leave a Reply

Your email address will not be published. Required fields are marked *

You may use these HTML tags and attributes: <a href="" title=""> <abbr title=""> <acronym title=""> <b> <blockquote cite=""> <cite> <code> <del datetime=""> <em> <i> <q cite=""> <strike> <strong>