Code Review

Reviewee

  • review requirements
  • verify all code is checked in
  • review code diff
  • test plan
  • deployment plan (if needed)

Reviewer

  • review requirements
  • check code out
  • build, test, and run it
  • follow happy path
  • is test plan sufficient
  • audit for maintainability
  • audit for robustness
  • is deployment plan sufficient
  • is logging and monitoring sufficient

Notes

  • purpose of code review: quality code, transmission of knowledge (good practices to reviewee, state of code to reviewer)
  • reviewing code as part of the career path of software developer
  • design review vs. code review
  • keeping scope small
  • must have and nice to have
  • PR riders: some refactoring to make the PR easier acceptable, but unrelated riders should be rejected
  • walk thru
  • reviewee checklist and reviewer checklist
  • reviewee responsibilities vs. reviewer responsibilities
  • does the code need to be polished and ready to deploy before submitting for review
  • what the reviewee should do (train the reviewee to do these things)
  • enforcing standards
  • static analysis: syntax errors, undeclared/unimported identifiers, dead code
  • immutable style, eliminating global state, data denormalization, cache key collisions
  • encapsulation, responsibilities
  • do names make sense (consistent, meaningful to end user, much could be said about this)
  • check out, build, test, run
  • correctness, test plan
  • error handling: branches, exceptions, return values, all possible values (especially null or zero/empty values), input validation, error handling syscalls, file system, external
  • security audit
  • JPL C code standards
  • SQL correctness
  • acceptance testing; meeting
  • is logging sufficient; are logs being rotated; where will I find the stack trace
  • deployment plan; performance (external services); load testing; canary testing; do multiple services or database changes need to be deployed in parallel and if so is this addressed; can change be safely rolled back
  • how long should a code review take: (review is timeboxed, audit, which attempts to enforce quality standard is not)
  • mandatory and optional changes (I would have done this differently)
  • reducing latency: having a separate tree checked out for code reviews
Unless otherwise stated, the content of this page is licensed under Creative Commons Attribution-ShareAlike 3.0 License