Code Review Checklist

The following is an aide memoire of some of the items that should be checked during code reviews. It should be updated when we discover problems that should have been caught by the code review. It may also serve as a list to check against before a pull request is submitted.

Comprehensibility of the code.

• Is the code readable? (Indentation, variable names, etc.)

• Is the code simple? Was this optimization really necessary?

• Does the patch contain commented out code? If so why?

• Are the public interfaces commented?

• Does it have a documentation impact? If so, does the pull request contain the documentation?

• Are there test cases with the code? If not, why not?

• Does the pull request mention which tests were performed?

Structure of the code.

• Could some of the code be commoned up or use existing functions instead?

• Does it have the right level of encapsulation?

Functionality of the code.

• Could it leak memory (or other resources), or free memory too early?

• Are there any dangling references to objects which remain after an object has gone out of scope? (Seems to often happen with StringBuffer.str().)

• Any access to uninitialised variables?

• Is the code thread safe? Unprotected access to shared variables? Deadlock/livelock?

• Does it correctly handle error conditions including exceptions?

• Does it correctly handle boundary conditions?

• Could it introduce any security issues?

• Does the patch contain any relevant test code?