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.

General questions

• What impact does it have on documentation? Is there a JIRA?

• Has the JIRA been updated to describe the issue, and the solution.

Testing

• Does the pull request contain tests, or details of how it was tested?

• Is a new unit test appropriate?

• Has the compiler and performance regression suite been executed?

• Has the LN internal regression test been executed?

Compatibility

• What could this change break?

• If fixing a bug, could people be relying on the old behaviour?

• Do we need to reference this change in the Redbook?

• Have we changed any interfaces that will break existing code? Do any interface versions need to be increased?

• Is it targeting the right branch? Does the target build in the Jira ticket match the target branch? Is the level of change appropriate for the release being targeted?

Miscellaneous

• Is the commit message free of typos?

• Does the commit message title reference the correct Jira ticket?

• Are user (error, warning & informational) messages comprehendable (non-cryptic)?

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 and functions commented?

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 via pointers that could potentially be NULL?

• Any access to uninitialised variables?

• Check any const_casts - are they really valid?

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

Style:

• Does it use the accepted format for //MORE comments?

• Does it introduce any trailing spaces? Does it contain tabs rather than spaces?