Code Reviews

Code reviews stand as a pillar for maintaining our standards of excellence in what we build. They serve to confirm that the code we endorse meets our high-quality benchmarks. The focus of a review is to evaluate the quality of the code itself rather than its functionality. It’s the responsibility of the code’s author to ensure its effectiveness and resolve any functional issues, while the reviewer’s role is to scrutinize the code’s structure and adherence to best practices. To safeguard the integrity of our production environment, every code segment destined for release is required to be vetted by at least one other developer. Although code changes that don’t interact with actual client data might be directly committed to the develop branch without a formal review, it’s still advised to have these changes reviewed to uphold our quality standards.

​

Types of Reviews

We have three ways to review code at Trew Knowledge:

1. External Reviews look at plugins from outside sources we’re thinking of using.
2. Internal Reviews check our own code before it goes live.
3. In-Depth Reviews take a closer look at complicated features.

Here’s how they differ:

• We’re tougher on code quality in internal reviews but focus more on checking for security and how well something runs in external reviews.
• Even though we trust our internal code more, we still don’t compromise on checking its security and performance.

Finding a plugin that works well but is messy code-wise? We might look for something else because messy code can be hard to manage later on and are a code-smell^{[1] [2]}. If a plugin has a large development team that is active, this can outweigh the code quality factor, but it’s always worth checking. We prefer to review code in small pieces through pull requests. It makes it easier to see details and give better feedback. But, this way, we need to be careful to keep track of how each piece fits into the bigger picture. If you’re working on a tough part and want a second pair of eyes, just ask. Always ask questions if something doesn’t make sense in a review. It’s better to clear things up early than to have problems later when the code is already being used. When we review code from clients, we treat it like our internal code because we want our clients to work to our standards. But, we might let some small things slide(like styles) as long as there’s no risk to security. With client code, the main thing is being practical and making sure everything is safe and up to par.

​

Factors to Review

​

Security

Security tops the list when we review code. Every piece of code, whether for production or staging, needs to be ironclad in terms of security. For detailed instructions on how to keep your code secure, refer to our frontend security and backend security standards.

​

Performance

Performance matters a lot because of the size of the sites we handle. Figuring out if something is performing well can be tricky, and usually, it’s up to the person who wrote the code. But, checking performance is also important for the reviewer, especially with external plugins that might not be built for speed. To get more details on what to look for, check our guidelines on frontend performance and backend performance standards.

​

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

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.

​

Behaviour and Logic

When it comes to code reviews, we’re usually not looking at the code’s actual behavior. The idea is to check the work, not redo it. But, if you’ve got some complex logic and want a second pair of eyes, just say so in your pull request. It’s way better to be safe than sorry, especially if there’s a chance something might not work as intended. The best people to ask for these thorough checks are your teammates working on the same project. They’re more likely to quickly understand if the code does what it’s supposed to. If you’re making changes to how things look on the site, throw in a before & after screenshot or a screen recording in your pull request description. It helps the reviewers see what’s changing and keeps everyone in the loop about new features or user interactions. For screen recordings, if you’re on a Mac, you’ve already got the tool built-in.

​

Automated code reviews.

• All projects should use the WordPress-VIP PHPCS standard. The easiest way to install phpcs and the ruleset is using Composer.
• Some projects use CircleCI or Github Actions to run coding standards checks.
• All automated checks should pass before requesting a code review from a human.

​

Requesting a Review from a Human

When your code is ready for review, it’s up to you to request the review. Generally is handled via a Needs Review or a Review & Merge label. Reviewers are expected to approve, reject, or comment on the pull request. Try to keep all communication in the pull request itself, so everyone can see what’s going on. If the reviewer is not sure about something, they will ask for clarification. Use Needs Review when you’re ready for a review but don’t want to merge yet. Use Review & Merge when you’re ready for a review and want to merge as soon as it’s approved.