Code Review Guidelines

Code Review Guidelines

Document Scope and Purpose

This document is intended to be a living document on delivery great quality software products and solutions. In order to deliver a positive experience this document acts as a minimum baseline and not a comprehensive checklist. Given different development environments, languages and architectures present in technology it simply would not be feasible to maintain a comprehensive how to or must have documentation. Instead we will focus on what makes quality software as general rules and practices. Clean quality code is self-evident, this baseline document helps those unfamiliar with delivering quality code a roadmap.

Purpose of code review

Code reviews first and foremost are to ensure a quality product is being delivered prior to deployment. Secondarily it ensures that the quality of the product meets the organization and team’s definitions of secure, effective and standardized product. Reviews are a method of positive accountability and teaching moments.

Code reviews should be conducted before code is introduced into production and change the environment.

Code reviews are not

A tool used to destroy a developer / team member, a team or a product in any negative fashion. Code reviews are about delivery quality product every time.

Code Review Participants:

Participants in a code review at a minimum should include the original developer(s), senior developer and a QA member if applicable to the team structure.

When should code reviews occur?

  • With each major function or application change key elements of the code should be reviewed before it is deployed or final check-in
  • Minor changes should be spot checked based on a statistical selection or “code coverage”
  • During all hotfixes or critical patches
  • Whenever a developer asks for assistance from another developer
  • As part of lessons learned from any Production failure or severe QA/UAT failures
  • As teaching moments to young developers from senior developers

What is reviewed for the following items:

  • Effectiveness of functionality
  • Review the business requirements vs work delivered
  • Ensure that the code fits the defined roadmap
  • Ensure that the code is secure and fits the team security standards
  • Ensure that the code base is checked in and compiles
  • Ensure logging and error handling is addressed

Code review items to check:

General Design / Execution

  • With a recent pull, does your code build and compile?
  • Does your code meet the minimum requirements for service design?
  • Is the code understandable?
    • Properly and clearly named variables
  • Clearly defined logic
  • Is code duplication avoided?
    • Do objects already exist that you can call?
  • Does your code offer extensibility?
  • Is the function / class / instance properly sized:
    • The function isn’t trying to do too much that will be difficult to maintain.
    • How long are the methods?
  • Has the code followed proper segregation regarding?
    • Data Access Layer
    • Business Layer
    • Presentation Layer
  • Are you using framework features vs writing custom code?

Security

  • Does your code meet the minimum requirements for the security standards?
  • Data sharing with third parties is in-line with data sharing agreement
  • Application code does not expose more data elements than necessary
  • Credentials are not saved in plain text
  • OWASP guidelines are followed for publicly facing endpoints

Documentation

  • Is your code documented both header, inline and story / tasks?
    • Inline:
      • Not just what you are doing, but why you are doing it.
      • Mention any future items that need to be addressed.
    • Header:
      • Are you using the established format for documentation?
    • Story:
      • Are you updating your tasks with current status?
      • Have you conveyed your logic into your story?
      • Have you updated the story so that QA and the business owner will understand your functions?
  • Are system diagrams updated to account for your application flow?

Testing

  • Can your code be tested / unit tested easily?
    • Have you tested your code?
  • Have you run through the acceptance testing that QA is going to run it through?

Error Handling

  • Error handling on all modified code / new code
    • Bubble errors up to main thread
    • Log errors to DB for debug later
  • Have you tried to break your code? Pass in incorrect values to trigger the error handling?
  • Are you logging the error to a central location?
  • Is production support aware of the error handling you have built?
  • Ensure that you are not logging sensitive information in a log table.