Code Review Checklist

Owned by Josh Zamor
Last updated: Feb 23, 2018 by Paweł Albecki (Deactivated)
2 min read


This checklist should be used by code reviewers to guide their feedback and by review authors in understanding what reviews will be looking for.  This is a living document and it should be updated by the Team Leads in a continual effort to improve our code base by improving our process.  Considerable effort should be made to keep this document concise and easy to work with; if a checklist item may be simplified or even removed by automation, the project should prioritize this to realize gains across our development community.

Clarity

  • Proper spelling
  • Conforms to Style Guide
  • Class, method & variable names are clear and concise
  • Test method names should clearly describe what is being tested and under what conditions (eg. shouldNotAllowXWhenYIsActive)
  • Class / method is not unreasonably long or complex
  • Gets the job done - and no more.  Extraneous types or concepts aren’t introduced because we think we might need them in the future

  • Dead code, useless comments, TODO’s, debug logging has been removed

Documentation

  • Javadoc exists for all public interfaces (Package, Class, Method)
  • RESTful interfaces are well documented in RAML. Every parameter and return has a JSON schema. JSON schemas are defined in their own JSON files and put in the schemas subfolder.
  • Documentation explains why something exists over how it works
  • Documentation covers edge cases and lays out the contract - success, error, exception.
  • README introduces the purpose of the services and guides developers in getting started.

Encapsulation

  • The package demarcations are useful - if package private scope is used, the contents of the package are still properly encapsulated
  • Classes and methods have private scope, unless they need to be package-private or public
  • Good OOD is used - message passing over get().get().get().
  • Interfaces expose exactly what’s needed, and no more.  e.g. db internals
  • Service boundaries are respected
  • Types are used appropriately.  e.g. use UUID not integer,  Integer instead of String
  • Util classes are not used – in OOP, object should contain both data and a behavior performed over that data. Regular classes should be used instead or behavior should be added directly to the already existing classes as non-static methods.

Resiliency

  • Unit tests exist and they adequately cover edge cases
  • Integration tests exist and they adequately test the interface contract (i.e. for RAML we have JSON schema's for parameters and returns)
  • All tests that are meant to verify searching or filtering, do so on a larger dataset than just what matches the search criteria (e.g. an IT that searches user by username should run with at least a few users with different usernames in the database)
  • Proper error handling techniques are used
  • Exceptions are only used when there is a programming error
  • External dependencies are brought in with care - is it mature, is it stable, does its functionality overlap with an existing piece
  • Log messages are useful in a service oriented context and utilize an appropriate level

  • All automated tests run & pass, tests aren’t removed unless appropriate

Security

  • Actions that need to be secured, are.
  • Rate limiting is considered


Reference

Reasoning & Example:  http://blog.fogcreek.com/increase-defect-detection-with-our-code-review-checklist-example/

Motech's checklist:  docs.motechproject.org/en/latest/development/code_review.html


OpenLMIS: the global initiative for powerful LMIS software