Versions Compared

Version Old Version 2 New Version 3
Changes made by

Yaw Nti-Addae

Yaw Nti-Addae

Saved on

Key

  • This line was added.
  • This line was removed.
  • Formatting was changed.

What is Code Review?

Code Review is the process throughout which source code gets assessed by one or more software developers or engineers. A thorough reviewer usually looks for inconsistencies, errors, potential bugs, problems in design and architecture, and issues of performance and security. Its benefits include an increase of collaboration, mitigation of risks, and a decrease of knowledge silos.

Info

Definitions:

Author: Developer that produced the source code

Reviewer: Developer assigned to review source code

Tech Lead: Technical manager of author

Review Master: Developer in charge of code review activities

Code Review Best Practices

Expand
titleBest practices for authors

  • Always add some business context, so your team can understand why this code is being added, changed, or removed.

  • If it's am incomplete work, add some checkboxes or list with the missing tasks to the description.

  • Enrich your description with visual content, like gifs, screenshots, or diagrams. Images and videos accelerate the understanding of the change.

  • Defend any arbitrary decision before the discussion to ignite. For instance, if changes are not following a convention, tell your team why you opted by this solution before they point it out. It focuses on the comments and avoids overwhelmed communication.

Expand
titleReviewers guideline

  • Does the code work? Check whether function and logic are correct.

  • Are functions, methods, and variables adequately named?

  • Are names semantic and meaningful for your business?

  • Is it well tested?

  • Are there unit tests, and they have good quality? Do they test the functionality?

  • Do tests reach acceptable coverage for your project?

  • Is the code clear and easy to understand?

  • Does it meet the team's guidelines and style guides?

  • Is there any duplicated code? Remember that duplication is far cheaper than the wrong abstraction.

  • Is there any commented or unnecessary code?

  • Does the code take the most out of frameworks and language? Is there any custom implementation of native or already-existing functions?

  • Are there code smells, grey areas, or bug-prone?

  • Is documentation on functions, methods, classes, contexts, and behaviors adequate?

  • Is the code as modular as possible?

  • Are the critical spots adequately logged?

  • Does the code consider failures? Is it just considering the happy path?

  • Are there better or simpler solutions?

  • Is there any performance issue?

  • Are input data sanitized?

  • Is there any SQL Injection point?

  • Is sensitive information being encoded or encrypted?

Code Review Checklist

Expand
titleGeneral

  • Does the code work? Does it perform its intended function, the logic is correct etc.

  • Is all the code easily understood?

  • Does it conform to your agreed coding conventions? These will usually cover location of braces, variable and function names, line length, indentations, formatting, and comments.

  • Is there any redundant or duplicate code?

  • Is the code as modular as possible?

  • Can any global variables be replaced?

  • Is there any commented out code?

  • Do loops have a set length and correct termination conditions?

  • Do the names used in the program convey intent?

Expand
titlePerformance

  • Are there any obvious optimizations that will improve performance?

  • Can any of the code be replaced with library or built-in functions?

  • Can any logging or debugging code be removed?

Expand
titleSecurity

  • Are all data inputs checked (for the correct type, length, format, and range) and encoded?

  • Where third-party utilities are used, are returning errors being caught?

  • Are output values checked and encoded?

  • Are invalid parameter values handled?

Expand
titleDocumentation

  • Do comments exist and describe the intent of the code?

  • Are all functions commented?

  • Is any unusual behavior or edge-case handling described?

  • Is the use and function of third-party libraries documented?

  • Are 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’?

Expand
titleTesting

  • Is the code testable? The code should be structured so that it doesn’t add too many or hide dependencies, is unable to initialize objects, test frameworks can use methods etc.

  • Do tests exist, and are they comprehensive?

  • Do unit tests actually test that the code is performing the intended functionality?

  • Could any test code be replaced with the use of an existing API?

Warning

Authors:

  1. Unit tests must accompany all new and modified code

  2. Unit tests must be modified to reflect modified or removed code

  3. NO code should be merged to “Develop” without a sign-off from Reviewer and explicit approval from Tech Lead and Review Master

  4. Any test data used for developer testing should be deposited and annotated in the Test_Data repository before sending code for review

Warning

This document should NOT be modified without the explicit approval of the Product Manager

Info

Acknowledgements and more information

SourceLevel

SmartBear

NYU-CDS