Code Review Checklist

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.

• 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

• 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.

• 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.

• 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

• 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