code review checklist

checklist.md

Code Review Checklist

With inspiration from Liberty University, https://wiki.openmrs.org/display/docs/Code+Review+Checklist, http://www.codeproject.com/Articles/593751/Code-Review-Checklist-and-Guidelines-for-Csharp-De.

For all configuration file changes:

• [ ]

For all code changes:

Structure

• The code makes sense
• Correctly and completely implements the design
• Conforms to pertinent coding standards
• The code is well-structured, consistent in style, consistenltly formatted
• Free of uncalled, unneeded, or unreachable code
• Free of repeated blocks of code that should be moved to a function
• Free of "magic number" constants where possible
• Complex procedures have been split into modules that are not too long

UI

• All user-facing strings are translated

Documentation

• The code is easy to read and has clear intent
• Comments were added only where additional clarity is required
• Any new comments added are correct and consistent with the code
• Any existing comments have been updated to be consistent with the new code
• Changes that are non-obvious work-arounds or performance optimizations are commented
• Free of commented-out code (we can always get it back from version control!)

Variables

• Variables are properly defined with meaningful, consistent, and clear names
• Have propre type or casting
• Free of redundant or unused variables

Arithmetic Operations

• Avoids comparing floating point numbers for equality
• Prevents rounding errors
• Avoids additions and subtractions of floating point numbers with greatly different magnitudes
• Any divisors will never be zero, or are checked for that case

Loops and Branches

• Loops, branches, and logic constructs are complete and correct
• Every case statement has a default
• Loop termination conditions are obvious and always acheivable
• Any statements that do not need to be inside the loop are moved outside the loop
• The index variable is never manipulated inside the loop
• The index variable is not used after exiting the loop

Tests

• Any new features are accompanied by unit tests
• If the change fixes a bug, a regression test is included for the bug

Java-specific:

• Something about avoiding dereferencing null pointers? :)

javascript-specific:

• The code lints without error
• Any use of parseInt is checked with isNaN
• Free of any == (equality comparrisons should always use ===)
• Calls to external libraries are wrapped in try/catch (except obvious ones like underscore, backbone, ...)
• try/catch is avoided in performance-critical code paths
• Manually storing, passing, and manipulating DOM elements is avoided where possible
• AJAX requests (and all promises that can fail) handle failure

SQL-specific:

• [ ]

CSS-specific:

• selectors use classes where possible (avoid using IDs)
• the order of rules in the ruleset is consistent
• if a preprocessor is used: the code is free of hard-coded colour or other values that should be in variables

For any new file added:

• The new file is placed in the correct location in the project
• A copyright attribution to DG is included near the beginning of the file

For any third-party library added:

• The library adds enough value to justify its addition

• Our use of the library meets any license requirements of the library

• Javascript and CSS for Dashboards and GIS:

  • The library was added as a dependency in the module's package.json, or there is a good, documented reason for copying its source.