Code Reviews

Code reviews ensure the quality of what we’re building remains high, and that code we’re vouching for remains up to the same standard. Reviews are primarily about assessing the code quality, not necessarily the behaviour. The code author is the one responsible for writing the code and making sure it works, while the reviewer is there to review the code.

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

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

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.

There are a huge number of factors that must be considered with security, but they mostly boil down into a few key things:

  • Never trust user input

    User input can never be trusted, regardless of the source. “User input” actually means anything that’s not written into the code itself, and includes things like external HTTP request data and translations.

  • Sanitise on input, escape on output

    Your database should always store a safe value, so all data coming into the database needs to be sanitised before saving. This could mean ensuring a value is a number, or that it only contains valid HTML. Sites also tend to be write-once, read-many, so moving the expensive sanitise step to when you write it can have better performance.

    This data also needs to be escaped for the relevant context on output. If you’re echoing into a HTML attribute, the code needs to be escaped for the attribute context; the same applies for text output into a HTML context. Escaping is always context-specific, so must be done at the last possible step, ideally immediately before output.

  • Check user authorisation and intent

    The capabilities required to perform a task should always be well-defined, and should be checked as early as possible. In addition, always check the user’s intent: did the user themselves trigger this action?

    The key tools in WordPress to achieve this are the roles and capabilities system, and the nonce system.

The 10up Best Practices guide and WordPress.com VIP Dcoumentation have great further detail on these points, and specific things to look for.

Performance

Performance is very important with the scale of the sites we work on. If it’s not performant, it cannot ship. However, it’s important to be pragmatic; performance is a rabbit hole that you can spend infinite time on. Shipping mostly-performant code is better than not shipping, and you can always incrementally improve it later.

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.

Typical factors to check for:

  • Unbounded queries

    It’s super easy to kill a large site by forgetting to put a limit on your query. Queries should always have an upper bound to ensure you don’t try and load 400,000 posts into memory.

    This can be tough to spot locally with only a few test posts, so it’s important to be careful about checking every query.

  • Uncached queries and requests

    Data should almost always be cached when it’s being loaded from an external source, whether that’s the database or an external HTTP request. Use either the existing caches in WordPress, or add your own caching code.

    Learn which functions in WordPress are uncached and which are cached, or ask for help.

Again, the 10up Best Practices guide contains specific examples, however this advice should be considered against other factors in reviews. As an example, the 10up Guide recommends avoiding in_array in most cases in favour of isset; while this does have slower (linear-time/O(N)) behaviour, in_array is more readable, and is not going to be a performance problem for arrays with <10k elements. Micro-optimisations like this should be tested, and only implemented if they make a significant difference over a more readable variant.

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. We’re working on improving this.

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

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 “Review” or “Review & Merge” 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 ask in the #code Slack channel with something like:

:pr: https://github.com/humanmade/the-project/pull/42

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

If no one is around to handle the review, you’ll 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.

You can then comment on the issue and ping them directly, typically with a message like:

@rmccue #reviewmerge

You should also assign the pull request to the reviewer.