Code Review Checklist

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


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