6 steps to do better code reviews

TLDR ‍

• Make everyone do code reviews, even junior folks
• Check for code readability
• Make sure the pull request is creating passive documentation
• Test performance
• Check for potential security risks
• Check testability

Disclaimer: This guide is for post Product-Market-Fit companies. If your company is pre-PMF, you should be optimizing for speed of iteration above anything else. If you’re pre-PMF your headcount should also be as small as possible, therefore a lot of the guidelines here don’t apply. Adapt according to your needs.

We’ve all seen something like this happen before.

We don’t blame you. There’s always pressure from management to ship stuff as quickly as possible, to ultimately allow the product to generate more revenue or traction. In fact that’s our end goal as developers.

However, having a culture of precarious or null code reviews leads to the accrual of technical debt.

‍

What is a code review?

A code review is the process of vetting code by another developer. A developer sends new code to be integrated to the main branch, and asks a teammate to review it.

The main goal of code reviews is to maintain a high code quality on the codebase. A second programmer can possibly identify issues in the logic, the readability, the modularity, the dependencies or the functionality of a programmer’s code.

Having a well established code review process also helps educate your team. It helps developers learn from each other.

So what’s the best way to make code review work?

‍

Make everyone do code reviews

Even the most junior developer should be reviewing the technical lead’s pull requests.

You might think this is counterintuitive. Wouldn’t this allow for a lot of pitfalls? Yes, but that actually leads to a benefit. A developer with a minor seniority will not necessarily understand everything about the code context, might not anticipate all the potential failures, and generally speaking will not catch as many errors as a senior developer.

This however, forces the senior developer to explain his code changes better.

There are 2 techniques to distribute how pull request reviews are distributed. GitHub, for example, provides the option to automatically assign a PR reviewer using both.

Round-robin focuses on distributing the number of reviews equally, regardless of the number of outstanding pull requests they have.

The load balancing algorithm focuses on distributing pull request reviews based on the number of outstanding pull requests each team member has. This algorithm tries to ensure that everyone reviews the same amount of pull requests in any 30 day period.

You can take a look at GitHub’s documentation to learn how to set this up for your team.

Both methods are completely valid, and which one to choose from depends on the specific characteristics of your team. We suggest round-robin if the seniority level of your devs is homogenous, and load balance if it’s heterogeneous.

‍

Check for readability

Code must be readable to be maintainable. Code must be maintainable to be useful for a software development team. Here’s a checklist for code readability.

• Are packages in order? Sometimes packages release new versions and they come with breaking changes. However, as Benjamin Silva suggests, embracing huge changes when they come is part of the deal. Spend a few days refactoring when big version releases come, and that way you will always have your packages up to date.
• Does the code adhere to your organization’s standards? The standards are very specific to each team and can be around naming conventions, linting, or generally speaking some kind of series of rules that people on the team agreed on.
• Are the variable and function names descriptive? Use intention revealing names, beware of names which vary in small ways, use pronounceable names, and use searchable names (eg: don’t name array indexes i and j, name them something descriptive like numberOfTasks).
• Can the code’s functions be broken up into smaller ones? It’s worth refactoring the code to achieve this. Does the code need comments? A priori, yes. ‍

Does the pull request include useful passive documentation?

• Title: Does the title give me information about the change? A good practice is to include the (Jira, Linear, Notion, ClickUp, or whatever ticketing system you use) ticket code on the title to link these 2 pieces of information. Another good practice in pull request naming is starting their title with an imperative present tense verb (eg: “move the module…, “create the function…”, “debug the feature…”)‍
• Description: Is the author explaining the problem? The intended solution? Is there a testing plan outlined? Does the description include a screenshot or a video?‍
• Is the type of change outlined in the description? Including this piece of information gives better searchability to your team. If you want to take a look at a pull request template, take a look at the one we have in our open-source repository.

‍

Test performance

How strict this should be depends 100% on the stage of the organization that owns the codebase. The testability requirements for a corporate, that has a well established revenue generating product, with millions of users; are different from the ones of an early stage startup that has the existential need of finding Product-Market-Fit, else they’ll run out of money. The former needs to optimize for strictness, while the latter needs to optimize for speed of iteration.

When you want to optimize for strictness, here’s what needs to be tested.

• Check for unnecessary code logging: Logging in the wrong place can significantly deteriorate performance and expose information that should remain private.
• Check data types: Based on the expected values, database types can be optimized for performance. For example in SQL, if the data varies in length, it can be more efficient to use varchar than char. If a column is going to store values between 1 and 5, use tinyint instead of int. Can performance be improved by adding or removing indexing? Depending on the context, this can be the case. Indexing doesn’t magically guarantee faster retrieval times. For example, if you break the First Normal Form property, indexing will make the performance decrease in some cases.
• Can you server-side render fetched data? Generally speaking, it is faster to make all the requests on a server than it’s to make extra browser-to-server round-trips for them. Server-side rendering is not always applicable, but when it’s, it should be there.
• Can queries return less data? At scale, returning too much data on a query will reduce performance.

Check for potential security failures

• Check for over-informative error prompts: Be informative, but not too informative. Sometimes these messages can give an attacker the information they need to break in. -** Check for exposed environment secrets:** Encourage good secret management practices in your organization. Once an environment secret is exposed on a remote branch, it will always be on the internet. It must be changed immediately.
• Make sure authorization and authentication are handled correctly: Always use https. Consider having different API keys for different roles and permission levels. ‍

Testability

• Are the tests passing? Whether they’re unit tests, end-to-end, or both… Make sure they are passing. If an already existing user flow changed, update the tests accordingly.‍

Originally published at https://www.watermelontools.com.