Code Review

Owned by Yaw Nti-Addae
Last updated: Dec 06, 2019
5 min read

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.

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

  • 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.

  • 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

Functions

  • Functions contain at most 5 parameters

    • God functions should be broken down. Parameters can be stored in related objects.

  • Functions should be <100 lines long

    • Separate out related actions into helper functions or classes

    • Properly encapsulate different conceptual levels

    • Functions should do one thing well, instead of several things poorly

  • At most one violation of the Law of Demeter.

    • Functions should not know anything about their super-super class, or implementation details nested in a subclass

    • Objects should be stored in variables before used, with exception of 'builder pattern' (eg. return stringBuilder.append(s1).append(s2).toString() )

Variables

  • No Global Variables without good reason

  • Unnamed constants only when obvious meaning without reuse

  • Variable names express the variable's purpose

  • Do not store two different concepts in the same variable

  • Avoid using invented terms when possible 

    • (eg. Don't name a variable "Computational Velocity" when it actually represents "Time since start")

  • Variables names should be not too short (mkr) as well as too long (markerUsedByPrincipalInvestigatorsOrLabStaff)

  • Variables are of appropriate type

    • Integer data is not in a string

    • Data Types should be appropriate to the stored data. In Java, primitive and non-primitive types can have different favored operations, and the appropriate use of transformation functions (eg, StringBuilders) will improve performance.

    • Avoid using non-specific types (eg, java.lang.Object) when type is known or expected

  • Filesystem refererences should be relative to an abstract root directory.

Classes

  • Classes should have appropriate author documentation

  • Significant modifications to the class should add new  'at author' tags.

  • Classes should be in appropriate folders

    • Remember class-name can exclude things in context of its path

      • e.g. 'Extractor.vcfExtractor.VCFExtractor has duplicated both 'vcf' and 'extractor', but the Hungarian notation should not be encouraged here.

  •  Classes should not duplicate responsibilities found in other classes.

  • Consider Composition over Inheritance

    • Inheritance should never be used as a 'method library'.

Packages

  • Only import classes you are going to use in your program.

  • Excess imports can lead to confusion as to which class is being called.

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

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

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

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