Code Review Checklist - rorymacleod/Toolbox GitHub Wiki

Automatable

These checks should be implemented as part of the build and enforced automatically.

  • Solution builds with no warnings.
  • Code meets all lint/static analysis rules.
  • All automated tests pass.

Test-driven development

  • All ACs have been transferred from the work item to the acceptance test suite.

Database Schema

  • All strings have a fixed length, unless there is a genuine reason to use nvarchar(max).
  • Foreign keys are indexed.

Migrations

  • The down migration completely reverses the up with no data loss.
  • An existing migration is never removed or renamed.

Entities

  • Foreign key properties corresponding to any navigation properties (e.g., OrderId and Order) are defined.
  • A test data generator is provided for new entities and properties.

Error handling

  • Exceptions are used for error handling, rather than return codes.
  • Custom exceptions deriving from AppException are thrown, rather than Exception.
  • Custom exceptions are serialisable and implement GetObjectData and the SerializationInfo constructor correctly.
  • Any parameters that would be useful for debugging have been added with SetData.
  • Exceptions are caught, logged, and presented to the user in a single, top-level, handler.
  • Exceptions are caught within the application only where additional context can be added.
  • Exceptions are re-thrown using throw;, not throw ex; or throw new Exception(...); in order to preserve the original call stack.
  • Any error message passed to AppException is intended to the displayed to the user.

Logging

  • Log messages are designed to be picked out and read from a long column of messages in a table:
    • The most important details that identify the type of message come first.
    • The wording is clear, but terse.
    • Similar message types use similar wording.
  • There should be a single log entry for any successful action that changes the system's state.

SPA Pages

  • Every page has a URL that includes any parameters required to restore the page's state in the query string.