code reviews

If you are assigned to a pull/merge-request or otherwise asked to review some code, please read all of the changed code and provide useful and specific feedback about correctness, performance, style, and convention.

All code that gets merged into ASGarD must go through a code review by one or more developers who are not authors of the proposed changes, and almost all pull requests are expected to have some number of requested changes.

Code Review Checklist

• Structure
  • single logical feature
  • if it is a bugfix
    • specific test(s) are added that would have caught the bug
    • all potential locations/occurrences are fixed
  • physical placement: appropriate component, directory, etc.
  • dependencies are managed well and minimized as much as possible
• Correctness
  • it is const correct
  • assert()s are used for function pre and post conditions
  • all return values are checked and handled
• Testing
  • all code paths are tested
  • test golden values are independently produced (and automated)
• Readability
  • self-documenting code: descriptive variable/function/class names, etc.
  • clang-formatted
  • functions and data structures are appropriately sized and scoped
• Documentation
  • comments exist and describe the intent of the code?
  • Are all functions commented?
  • any unusual behavior or edge-case handling is described
  • the use and function of third-party libraries documented
  • data structures and units of measurement explained
  • Is there any incomplete code? If so, should it be removed or flagged with a suitable marker like ‘TODO’?

Refer to the Thoughtbot guide to code reviews