Code review best practices

Code reviews are the best way to collectively improve the quality of our code while improving our skills as developers in the process. Be Humble, Be strict but Be Fair. Code reviews are not punitions, they are a gift and the opportunity to exchange ideas and communicate on code.

Put the "ego" apart

Not so easy right ?! Reviewing the code someone produced, watched it, check it, test it and and give a feedback on it. Here are a few tips on the posture to adopt to make a great review:

Be unbiased. Reading a peers code is already hard, so don't put your own emotions into the mix. When you don't feel good report it or transfer to someone else. Also, if you're to attached to the piece of code being changed, acknowledge the change first (see: The one ring effect).
Be impartial. You are reviewing the code, not the person who produced it. No matter how nice and handsome that person might be, you need to be impartial. It's your role to make sure the whole codebase wont be impacted, short to long term. This new code will have to be maintained, debugged, tested... so it better be easy to.
Share your experiences and learning, by transmitting the things you already tried in an other context which worked or failed.
Don't produce feedbacks using your code, the thing you've done or other personal targeted comments. The code is the responsibility of the whole teams so make sure to mention the code, this piece of software, we rather than the person's works.

💡 It's not your code, it's the teams one. The team might change, and it's the responsibility of the current members to ensure it's viable independently of "how bad it was you received it".

Principles

Make sure these principles are applied.

KISS

Keep it simple stupid

In software design Simplicity > Complexity Complexity in development yield bloated, slow and unmaintanable code. Keep it simple.

YAGNI

You aren't gonna need it

Do not implement things until you actually need them. Trying to foresee the future and create features before they are needed will only lead to awkward code or worse, code which will never be used.

LIFT

Locating our code is easy
Identify code at a glance
Flat structure as long as we can
Try to stay DRY (Don’t Repeat Yourself) or T-DRY

Naming

Naming things is hard, yet some simple rules can be used to make it better. It applied to class, variables, properties, methods, functions and even files or directory.

Use explicit names

Use explicit names, the length of the variable is not important, most of the time it will be mangled at the compilation.

// Do
const companyUsers = await company.getUsers();

// Don't
const a = await c.getUsers();

Function names should include a verb and a noun

The Verb should let us know exactly what the function is trying to achieve.

Always prioritize the "output" when you choose a noun. Why? Because outputs are not named in many programming languages, while inputs are.

// Do
createUser() // this function will clearly create a new user
signCertificate() // this function will sign a certificate
 
// Don't
get() // missing a noun! get what??
doContact() // do what exactly? what does do mean?
UserLogin() // verb should be the first thing we see: loginUser()
loggedUser() // functions are actions, not events. The past tense makes no sense and is confusing
manageSchedule() // verb is too broad. Manage is like "stuff, thing, truc", what are we trying to do? check the schedule? optimize the schedule? control something in the schedule?

The name reflects the role

The name reflects what a construct does or contain. The person reading the code should be pleased, not surprised nor have to search the meaning himself.

// Do
function getCompanyById(id: string): Company { /*...*/ }

// Don't
function get(ref: string): Company { /*...*/ }

Technical information or patterns are used

When useful, some nice implementation details can leak in the naming.

// Do
const count = 4;

async function getCompanyById(id: string): Promise<Company> { /*...*/ }

function* CompanyIterator() {
    while(companies.hasNext()) {
        yield companied.next();
    }
}

// Don't
// BAD: Postfixed with type
const countInt = 4;
// BAD: Postfixed with type
function getCompanyByIdAsync(id: string): Promise<Company> { /*...*/ }
// BAD: Looks like an array
function* CompanyList() { /*...*/ }

About booleans

When using boolean, name it using the positive value.

// Do
interface Finger {
    isMajor: Boolean;
}

// Don't
interface Finger {
    isNotMajor: Boolean;
}

📝 this example could work equally well as a Human, Scholar or Military interface.

Complexity

There are two kinds of complexity, algorithm complexity described by the Big O notation and the mental complexity being the mental load you need to unravel to work efficiently. It's generally a good idea to maintain those levels as low as possible.

ℹ️ It is not very pleasant to work with a method of 500 lines which takes 14 parameters and which mutates 23 fields.

Explicit arguments

Check the methods for there arguments specification. It should :

Avoid boolean traps (the last parameter being a boolean which change the behavior of the method),
Not having many unused parameters,
Be descriptive on it's arguments and there roles,
Don't mix up things,
Use a typed and properly named definition.

// Do
const user = createUser({ name:“toto”, role:“admin” }, { sendEvent: true });

// Don't
// BAD: too many arguments, why so many null ?, what do they mean ?
const user = createUser("toto", null, null, "admin", null, true);
// BAD: Why the heck is sendEvent here ?
const user = createUser({ name: "toto", role: "admin", sendEvent: true });
// BAD: don't pass unused properties
const user = createUser({
    name: "toto",
    role: "admin",
    email: null,
    avatarUrl: null
}, { sendEvent: true });

☣️ It’s almost invariably a mistake to add a bool parameter to an existing function

💡 There are many good resources online on boolean trap and why they should be avoided: Trolltech, spice factory...

One purpose

Check the piece of code is only doing the minimum amount of work required to achieve a single goal. Trying to achieve multiple goals with one piece of code is a code-smell.

// DO
const user = createUser({ name:“toto”, role:“admin” });
const savedUser = await saveUser(user);

// Don't
const user = await createUser({ name:“toto”, role:“admin” }, { save: true });

💡 Smaller bits of code can be easily assembled to something bigger, are generally easier to read and understand and can be unit tested.

Mutations and side effects

Check the code is not reading or changing things outside of its own scope. Prefer a limited scope with predictable outcomes.

// Do
function multiply (initialValue: number) {
    return function(factor: number) {
        const value = initialValue * factor;
        return { value, next: multiply(value) };
    }
}
const multiplyBy4 = multiply(4);
const multipliedValue = multiplyBy4(20);
console.log(multipliedValue.value); //80
console.log(multipliedValue.next(2).value); //160

// Don't
let thingValue = 4;
function doThing(factor: number) {
    thingValue *= factor;
}
doThing(20); //80
console.log(thingValue);
doThing(2); //160
console.log(thingValue);

Cyclomatic complexity

It is a quantitative measure of the number of linearly independent paths through a program's source code. In short, each condition creates a new path in the code execution, each one can be the source of a bug and should be tested. This value encourages you to create very linear functions.

// Do
// Complexity = 6
const getEmbeddedUser = (embedded?: EmbeddedHttp, userId?: string): EmbeddedUser | undefined => embedded?.users?.[userId ?? ''];
// Complexity = 7
const getEmbeddedUserProp = (user?: EmbeddedUser, prop: keyof EmbeddedUser, { defaultValue = null }: { defaultValue: string | null }): string => user?.[prop] ?? defaultValue ?? '';
// Complexity = 1
export function toEmbeddedUser(embedded?: EmbeddedHttp, userId?: string): EmbeddedUser {
    const user = getEmbeddedUser(embedded, userId);
    return {
        id: getEmbeddedUserProp(user, 'id', { defaultValue: userId }),
        email: getEmbeddedUserProp(user, 'email', { defaultValue: userId }),
        fullName: `${getEmbeddedUserProp(user, 'firstName')} ${getEmbeddedUserProp(user, 'lastName')}`.trim(),
    };
}

// Don't
// Complexity = 24
function toEmbeddedUser(embedded?: EmbeddedHttp, userId?: string): EmbeddedUser {
    const user = embedded?.users?.[userId || ''];
    return {
        id: user?.id || userId || '',
        email: user?.email || '',
        fullName: user && `${user.firstName || ''} ${user.lastName || ''}`.trim() || ''
    };
}

💡 Some IDE plugins can help you calculate this like CodeMetrics for VSCode.

Algorithmic complexity

Every call has a complexity, a way to estimate the number of cycles of the call is by using the Big O notation.

map, foreach, reduce, filter, for : O⁽ⁿ⁾
find, indexOf : O^{(n log n)}
push, pop, shift, unshift : O⁽¹⁾

Check that the data structure is the right one to ensure the performances are met.

// Do
const users: User[] =  [/* 1 000 000 hypothetical users */];
const usersByBirthDate = users.reduce((acc, user) => { //O(n)
    if (!acc.has(user.birthDate)) {
        acc.set(user.birthDate, []);
    }
    acc.get(user.birthDate).push(user);
    return acc;
},new Map());
const birthDateTuples: [User, User] = usersByBirthDate.entries().reduce((acc, [, [first, ...others]]) => { //O(m)
    return others.map(otherUser => [first, otherUser]); //O(l)
}, []);
// Total O(n)+O(m*l)

// Don't
const users: User[] =  [/* 1 000 000 hypothetical users */];
const birthDateTuples: [User, User] = [];
for(const user of users) { // O(n)
    const otherUsers = users.filter(u => u.id != user.id) //O(n)
    for(const otherUser of otherUsers) { // O(n)
        if(otherUser.birthDate === user.birthDate) {
            birthDateTuples.push(user, otherUser);
        }
    }
}
// Total O(n³), about 1 000 000³ cycles
// Bug because data is duplicated

Logic error

Explicit casts are used

When a value is cast, make sure the cast is explicit, and that any developer not familiar with the language or codebase could read and understand it easily.

// Do
export const isConnected = Boolean(this.connectedUser);

// Don't
export const isConnected = !!this.connectedUser;

☣️ This sample uses the double !! notation. Event though it's logical and used on many codebase, It's not really developer friendly and not recommended.

The conditions are clear and understandable

Make sure that the role of every condition and what it does is easy to understand.

// Do
if (isConnected && hasRights(this.connectedUser, requiredRights)) //...

// Don't
if (isConnected, compare(this.connectedUser.rights, requiredRights).length === requiredRights.length) //...

💡 In these sampled, hasRights is far easier to read, but is also unit testable. Even though compare(/*...*/).length === requiredRights.length could be the proper implementation, it's hard to understand and must be abstracted away.

The result is predictable (endless loop...)

Check that loops are really necessary in the context (can it be replaced by a safer map, filter, reduce instead?) and that they correctly implement an exit condition.

// Do
const userIds = users.map(({ id }) => id);

// Don't
const userIds = new Array(users.length);
for(let i = users.length; --i >= 0;) userIds[i] = users[i].id; // At first glance, are you sure this loop will exit ?

💡 Those two piece of code do exactly the same thing. Even though the first is shorter, it's also testable and easy to read. Read more about those methods here : Array static methods, Array instance methods.

Condition should short-circuit

Check the condition is testing the smallest CPU consuming items first and that a short-circuit operator is used to prevent from a complete operands interpretation. In the following case isConnected is O⁽¹⁾ and hasRights is O^(n*m).

// Do
if (isConnected && hasRights(this.connectedUser, requiredRights)) //...

// Don't
if (hasRights(this.connectedUser, requiredRights) && isConnected) //...

💡 Read more on condition short-circuit mechanisms here : MDN.

Language errors

Generally everything in the code is written and named in english. Start by checking that everything is proper english, that they're no spelling mistake and that no native language (eg french) is in. Then, the thing must make sense, based on the context, it's role, the thing it represent.

// Do
export const AUTH_LEMONLDAP_COMPANY_NAME = 'authentication/lemonldap/companyname';

// Don't
// BAD: Not english
export const AUTH_LEMONLDAP_COMPANIE_NAME = 'authentication/lemonldap/companiename';
// BAD: Spelling mistake
export const AUHT_LEMONLDAP_COMPANY_NAME = 'authentication/lemonldap/companyname';
// BAD: Has nothing to do with the context
export const SMALL_SEA_HORSE = 'authentication/lemonldap/companyname';

💡 If you need help an extension like Code Spell Checker can be used.

Testability

Check that the code written can be tested, if it can, check that tests are present and that they're valuable and they covers the natural and edge cases. If it's not testable, then ask two questions :

Is it worth testing it ?
How should the code be changed to be testable ?

Here is a flow chart of how to check for testability.

Edge case

Looking the code and testing the application, think about the weird ways it might break.

Race condition
Non resolving promises
Something called before the initialization is done
...

What happens if at runtime :

The type given by typescript does not match the actual data ?
A property is empty or the interface with the server changed and the returned data are partial ?
The user only uses a tactile interface and the code expect mouse events.
The browser is not compatible.
...

Documentation

If a feature check that a documentation is present, on how to use/configure the thing, how to read the code, how it's built, the particular dependencies...

Patterns

For the common programming problems they're already existing solutions, under the form of libraries or of patterns to implements yourself. These are best practices easily identifiable, making the intentions of the programmer obvious.

Separation of concerns (SoC)

The code is supposed to do one thing and have only one purpose. If that's not the case, think about the delegation patterns.

// Do
import { EventEmitter } from 'events';

class Company extends EventEmitter
{
    private _name: string;

    constructor() {
        super();
    }

    public set name(name: string)
    {
        this._name = name;
        this.emit('change');
    }
    //...
}

const company: Company = //...;
company.on('change', renderer.update)

// Don't
class Company
{
    private _name: string;

    constructor(private renderer: Renderer) {}

    public set name(name: string)
    {
        this._name = name;
        renderer.update();
    }
    //...
}

☣️ The observable pattern is subject to memory leaks. This is the case for Node.js and Rx.js. Remember to unsubscribe after use.

Coupling

Can a dependency be swapped with a mock or any other implementation ?

// Do
class CustomerRepository
{
    public constructor(private readonly database: IDatabase) {}

    public void add(customer: { name: string })
    {
        this.database.addRow("Customer", customer.name);
    }
}

interface IDatabase
{
    void addRow(table: string, value: string);
}

class Database implements IDatabase
{
    public void addRow(table: string, value: string) {}
}

// Don't
class CustomerRepository
{
    public constructor(private readonly database: Database) {}

    public void add(customer: { name: string })
    {
        this.database.addRow("Customer", customer.name);
    }
}

class Database
{
    public void addRow(table: string, value: string) {}
}

💡 By simply using an interface you reducing coupling drastically.

Injection

What are the dependencies of the code and is it replaceable ?

// Do
import curry from 'lodash/curry';
import { CompanyServiceInterface } from '../services/CompanyService';

const getAge = curry(async (companyService : CompanyServiceInterface, companyId: string) => {
    const company = await companyService.getById(companyId);
    return Date.now() - company.foundedAt;
});

// Don't
import CompanyService from '../services/CompanyService';

const companyService = CompanyService.getInstance();

async function getAge (companyId: string) {
    const company = await companyService.getById(companyId);
    return Date.now() - company.foundedAt;
}

💡 When working with function with dependencies like this one, make it curry-able, it will allow to prepare the calls by passing the config and just using the required arguments on call.

const companyService = MockCompanyService.getInstance();
const getCompanyAge = getAge(companyService); // Passing the first argument here
// ...
console.log(await getCompanyAge('123456')); // And the second later

Example

Considering this piece of code:

class CompanyClass {
    private logger: Logger;
    private _name: string;

    constructor(
        id: string,
        private service: CompanyHttpService,
        private logger?: Logger = DefaultLogger(),
    ) {
        this.init(id)
    }

    async init(id: string) {
        const raw = await this.service.get(id);
        this._name = raw.name;
        /* ... */
    }

    set name(name?: string) {
        this._name = name;
        this.logger.info('Name has changed');
    }

    get name(name?: string) {
        return this._name;
    }
}

The constructor triggers an async behavior
Requires a service and a logger

class Company extends EventEmitter {
    // Separation of concern with an instanciator which also manages the async behavior
    public static async getCompany(id: string) {
        const rawCompanyData = await companyService.get(id);
        const company = new Company();
        company.init(rawCompanyData);
        return company;
    }
    // Using ES2020 private fields
    private #name: string = '';
    // Synchronous constructor
    private constructor() {
        super();
    }
    // Synchronous init since the async pattern has been moved await
    private init(rawCompanyData: RawCompanyData) {
        this.#name = rawCompanyData.name ?? '';
        /* ... */
    }
    // Removed the logger dependency by using the observer pattern
    public set name(name?: string) {
        this.#name = name ?? '';
        this.emit('changed:name');
        this.emit('changed');
    }

    public get name(name?: string) {
        return this.#name;
    }
}

const company = Company.getCompany();
company.on('changed:name', () => logger.info('Name has changed'));

Security

Do not trust user input

All user input should be extensively validated by the server. Client-side validation does not count since clients can easily be altered. Never trust anything which can be altered by a user.

For nodeJS, use well-known and tested libraries like joi or validator to check user input. Avoid coding/inventing your own: no matter how talented you are you will make more mistakes than the community.

When validating user input, always take the whitelist approach: define the allowed values instead of trying to identify a blacklist which will inevitably be incomplete and thus flawed.

Further reading: Owasp Input validation cheat sheet

Know the OWASP top 10

SQL Injection, XSS, CSRF... Best way to defeat your enemy is to know your enemy! The top 10 owasp compiles the most well known attacks your application could be facing. Know them, study them and don't get "PWND"!

💡 Further reading: https://owasp.org/www-project-top-ten/

Avoid default values/mechanisms when dealing with

Using default values for variables tied to authentication mechanisms is a terrible idea. If a parameter is not passed to your application and you revert to its default values you may end up exposing it to some serious security flaws. For instance, a default value on a secret may cause your live application to use a default which is too weak for production. Or worse, if the default is to by-pass security, your application may be left completely exposed.

Error handling

Handle errors properly and do not leak important information (kind of stack, versions, nature of the DB...). Has a general rule, exposing stack trace information should never be available publicly.

Definitions

Code-Smell

In computer programming, a code smell is any characteristic in the source code of a program that possibly indicates a deeper problem. Determining what is and is not a code smell is subjective, and varies by language, developer, and development methodology.

Here is a talk from Elijah Manor called eliminate JavaScript code smells explaining what it looks like in the Javascript universe.
Source making has a nice section on refactoring various Code Smells.

Photo by Startup Stock Photos from Pexels

Code review best practices #

Put the "ego" apart #

Principles #

KISS #

YAGNI #

LIFT #

Naming #

Use explicit names #

Function names should include a verb and a noun #

The name reflects the role #

Technical information or patterns are used #

About booleans #

Complexity #

Explicit arguments #

One purpose #

Mutations and side effects #

Cyclomatic complexity #

Algorithmic complexity #

Logic error #

Explicit casts are used #

The conditions are clear and understandable #

The result is predictable (endless loop...) #

Condition should short-circuit #

Language errors #

Testability #

Edge case #

Documentation #

Patterns #

Separation of concerns (SoC) #

Coupling #

Injection #

Example #

Security #

Do not trust user input #

Know the OWASP top 10 #

Avoid default values/mechanisms when dealing with #

Error handling #

Definitions #

Code-Smell #