Effective Code Reviews

Posted on Sat 24 February 2018

Code reviews are a pivotal part of the practices followed by engineering teams. It has many benefits that are mostly obvious but others not so much, In my experience I have seen reviews abused and revered under different circumstances. In this article, I'd like to cover the following:

  • Purpose of a code review
  • Potential outcomes of a review
  • Code of conduct
  • Anatomy of a good review

Roles

  • Reviewer: The person who is reviewing the code.
  • Reviewee: The person whose code is reviewed.

Purpose of a Review

The purpose of any good review is simply to ensure that only good quality code makes it to production. It is a stage for enforcing any basic or extended guidelines which the team or organisation has agreed upon. It is also one of the best opportunities for a dialogue pertaining to the decisions that resulted in the final solution.

Potential Outcomes

A review can lead to at least 3 distinct outcomes:

  • Approval pending change request.
  • Approval pending discretionary change request.
  • Approval with general comments and opinions.

Code of Conduct

Reviewee

  • Value people's time, do not initiate reviews unless you are sure that the work is done.
  • Be communicative.
  • Be receptive of criticism and subjective remarks but correct them.
  • Be prepared that some comments will be subjective but call them out.
  • Ensure that comments are dictated by functional value.
  • Make sure requested changes add some value otherwise question them.
  • Feel comfortable asking for technical references for comments and suggestions.
  • Defend your design decisions when necessary.
  • Always be willing to offer and accept a reasonable compromise.

Reviewer

  • Value people's time, do not commit to review if you cannot actively engage with the reviewee.
  • Be communicative.
  • Avoid blunt criticism and subjective remarks.
  • Ensure that comments are not subjective.
  • Ensure that comments are dictated by functional value.
  • Be prepare to provide technical references when needed.
  • Question design decisions when necessary.
  • Always be willing to offer and accept a reasonable compromise.

Anatomy of a Review

When reviewing, a reviewer needs to ensure that they cover the following aspects: Code

  • The code should not introduce unnecessary complexity.
  • Readability is as important as functionality.
  • Where possible, prefer simplicity over complexity, unless there is some clear functional advantage.
  • Ensure that imports are sorted and cleanly categorised.
  • Ensure that code is tested where applicable.
  • Ensure that code is documented where applicable.
  • Ensure that code is logged where applicable.
  • When in doubt refer to language reference and other sources.

Testing

Ensure that tests are sensible. A little more than exercising the API, we should be testing edge cases etc. Ensure that the code coverage does not regress.

Logging

  • Does the code require logging and if so has it been implemented.
  • Does the code demonstrate a good level of logging.
  • Excessive logging can slow down code where as too little logging is not useful for diagnoses of failure.
  • Logs should be categorised appropriately.
  • INFO, ERROR, DEBUG should be used sensibly where applicable.
  • Documentation
  • Generally it is a good idea to have as much documentation as possible.
  • Documentation of complex methods in code is a requirement.
  • Documentation of everything else is discretionary but encouraged.
  • The documentation should conform to the team standards. RST, MD etc.

Module Dependencies

* Ensure that unnecessary dependencies are avoided at all costs.
* Ensure that the versions of dependencies are updated where applicable.

dev / practices