Code Reviews

Every piece of code we push into production must undergo review by at least one other engineer. Code may be pushed to staging without review if it is not touching real client data, but should typically be reviewed anyway.

Types of Reviews Types of Reviews

There are three main styles of review that we undertake:

  • External Reviews – Third-party plugins we’re seeking to use.
  • Internal Reviews – Standard reviewing process before code is deployed live into production.
  • In-Depth Reviews – Internal reviews, but for tricky features.

Each of these styles of review has different factors to check for, but generally speaking, internal reviews are stricter on code quality checks, while external reviews are stricter on security and performance checks. Generally speaking, internal reviews have a higher level of trust, so reviews can focus more on ensuring quality is high. With that said, security and performance must always be a factor regardless of source, as they are critical to the end product.

If an external review reveals a plugin is technically OK, but has low code quality, checking for alternatives is typically a good idea. These plugins are generally harder to maintain, and are a code smell. If a plugin has a large development team that is active, this can outweigh the code quality factor, but it’s always worth checking.

Internal reviews should generally be performed piecemeal on the project; in other words, they should be done on pull requests before merge. This provides a much smaller set of code to be reviewed, and allows more in-depth reviews. Larger blocks of code are harder to review fully. The flip-side of this is that it can be harder to see the larger picture of the whole project, so the reviewer should be conscious of how code fits into the context of the project. In-depth reviews can be done by-request when someone works on something and wants a closer look to verify what they’re doing is right.

Never be afraid to ask for clarification. If you spot something in a review that seems a bit weird, mention it. It’s easier to have a quick conversation in the PR comments to double-check, than to assume something is correct only to find the issue in production. Spending time writing an emergency fix and deploying it is much more expensive (in time, and potentially in revenue) than taking the time initially.

When working with code submitted by clients, differing levels of review may be applied. Generally speaking, we want to help our clients to think and work like us, so these reviews can be treated as internal reviews. Some client projects may necessitate different levels of reviewing, but should never be less strict than external reviews, as we need to vouch for this code. Style issues can be overlooked in these cases, but security can never be. Be pragmatic.

Factors to Review Factors to Review

Security Security

Security is one of the biggest factors involved in any review. All code that we push into a production or staging environment must be completely secure.

For specific guidelines, consult the frontend security and backend security standards.

Performance Performance

Performance is very important with the scale of the sites we work on. Determining what is or isn’t performant is tough, and this role mostly falls on the code author. However, performance should also be checked by the reviewer, especially for external plugins that may not have been created with performance in mind.

For specific guidelines, consult the frontend performance and backend performance standards.

Accessibility Accessibility

Our work should be usable and accessible for as many users as possible. Reviewers should check for obviously inaccessible patterns.

For specific guidelines, consult the accessibility best practices.

Style Style

Code reviews should usually check that coding style matches our coding style guidelines. If someone is intentionally ignoring a guideline, that’s fine too.

Internal reviews are typically the only ones that need style checking, as we’re not responsible for external plugins. The exception to this is if we’re specifically asked for it.

Style checks should mostly be automated, as it’s easy to miss when writing or during a review. This can be performed by the coding standards checker, and can be enabled on a repo using the linter bot.

Behaviour and Logic Behaviour and Logic

The actual behaviour of code typically does not need reviewing. Reviews are a process of checking work by others, not to duplicate that work again. Sometimes however, you may want your code double-checked, especially if you have some tricky logic.

Feel free to ask on any pull request for a more in-depth review; it’s always better to ask and not need it than to ship with something potentially broken. The best people to ask for these reviews are other people on the same project, as they can verify that the behaviour matches what they expect much more easily.

Requesting a Review Requesting a Review

When your code is ready for review, it’s up to you to request the review. The way to do this changes per-project, but generally is handled via a “Needs Review” label. This could also be via a kanban column in ZenHub or GitHub’s Projects view.

You’ll also typically need to find a specific person for your review. For projects under the humanmade organisation that don’t need in-depth reviewing, you can assign a special Reviewer needed label to the pull request. A bot will automatically post this into the #dev-code-reviews channel.

For projects under client organisations, you should instead message the #{client}-private room.

If no one is around to handle the review, you may need to pick a specific person to handle the review for you. This is usually someone else on the project, but you can ask anyone; if you can’t think of anyone in particular, either Ryan (@rmccue) or Joe (@joehoyle) can handle it.

Reviewers are expected to approve, reject, or comment on the pull request, but should leave merging the pull request to the PR author unless asked specifically to merge.