How we do code reviews at Alan

Photo by Shane Aldendorff on Unsplash

Writing software requires finding a good balance between competing imperatives: moving quickly, maintaining a high level of quality, taking time to focus on writing code, and spreading knowledge about our work. At Alan, we use code reviews as a tool for ensuring high quality and spreading knowledge, without introducing too much delay in our workflow.

Opening a Pull Request

We manage our code with git. We have a main branch that gets deployed to production and we work on code changes in branches. When a code change is ready, we open a pull request on Github to request that it be merged into the main code branch. Code review happens at this point.

We aim at making pull requests small and focused to make them easier to review. When we need to make big renames or code movements, we split them off from logic changes into a separate dedicated pull request.

Reviewers look at both the implementation and the impact of code changes from a functional point of view. For changes to the web front-end or to the mobile application, we attach screenshots or short videos showing the application before and after the changes. This makes the review simpler without having to run the code locally. More generally, we aim at providing the reviewers with the information that will help them do a quality review as quickly as possible.

When opening a pull request, we usually select one or two engineers.

• One should be proficient in the area of the code being updated.
• The other may be less proficient, giving them the occasion to learn about a new code area.

If the pull request is very short, e.g. a typo or documentation fix, we have the freedom to choose to not have it reviewed and merge it directly. This is about trust: Alaners are trusted to choose the best course of action for their work.

Review process

The reviewers will make comments, maybe request changes and eventually approve the pull request. Once at least one reviewer has approved it, the author is free to merge it themselves.

Sometimes, the reviewer makes some minor comments but approves the pull request nonetheless. The author is trusted to address these comments, or raise follow-up discussions before merging. If addressing the comments led to extensive changes, a new review can be requested. Once again: Alaners are trusted to choose the best course of action.

In terms of time frame, we expect pull requests to be reviewed within one business day. It is ok to send a reminder to the reviewers after half a day, and send more insistent pings as the business day gets closer to the end. Of course, engineers aren’t expected to review pull requests during evenings or weekends.

The role of the reviewer

From an engineer perspective, the reviewer is here to help you contribute valuable changes to our codebase. You may see them as a second pair of eyes helping you take a step back. They also help spread good practices across the code base. You can disagree with the reviewer and challenge their objections.

When we merely want to inform another engineer about a code change, we simply tag them in the pull request description.

As a reviewer, you are not required to comment on a pull request when reviewing: it is ok to just approve. You can also make comments prefixed with “nit:” (for nitpicking) for things you would have done differently, but for which you give the author the discretion of keeping or changing their version.

Focusing on things that matter

Reviewing pull requests takes time and we strive to get as much value as possible from it. We leverage tools and automation to get the low value parts out of the way:

• We use code formatters: no need to argue about line breaks anymore.
• We use static analysis tools to catch common errors and necessary cleanups: variables used before assignments, unused imported, etc.
• We use linters to eradicate risky code patterns, like the == operator in Javascript.
• We also use custom linting rules when we spot frequent mistakes that are easy to avoid (more about this here).

All of the checks run as part of our build process: code cannot be merged into the main branch if it does not pass every one. They are started as soon as the branch is pushed to git and are parallelized so they complete quickly. With fast results, the author has a chance to revise the code before human reviewers view the pull request.

Depth of the review

Even on the fairly clean code that our tools accept, we cannot expect the reviewer to catch every mistake. By default, here is what we expect from reviewers at Alan:

• They understand the motivation for the changes and the pull request description should help them.
• They understand how the changes were tested, whether it was manually or programmatically. It helps to be specific here: we usually give some detail on the test scenarios we considered. It will help the reviewers and serve as reference for the next engineer who touches the code.
• They do not find a major flaw in the technical design choices.
• They find the changes consistent with the usual practices of the community, or the crew. This includes naming conventions, code organization, specific implementation patterns.
• They see no obvious errors in the implementation.
• They find the code easy to understand to the extent possible.
• They could investigate problems related to the code, if something goes wrong.

We don’t expect reviewers to run the code locally and make sure it behaves correctly. We also try to avoid challenging minor implementation details that would make the code a little more this or that: it is too easy to waste time on this and rarely brings any value.

Of course, all these are default expectations. It is entirely accepted to ask questions to the reviewers, or ask them for more (or less) depth on certains aspects of the review.

Wrapping up

Overall, our code review process follows two themes:

1. We trust each engineer to choose the best approach to validate code changes.
2. We want to gain enough confidence that each code change is beneficial to Alan.

As a result, the depth of the reviews varies depending on the context: we move fast whenever possible and we double check the details for more sensitive topics.

Code review is just one of our tools that enable shipping high-quality products quickly. We use automation — linters, etc. — to make the process efficient, but that automation has a deeper role. We use it to spread consistency and common practices across the team. Our engineering community is growing and it is key to be able to learn from others’ mistakes and to be able to transition efficiently from one product team to another.