Code reviews are invaluable for the health of a codebase. During reviews, you can detect bugs, find opportunities to improve security and performance, and much more. But if you’re not using the best code review checklist, you could be harming your engineering efficiency unintentionally.
Our recent study from LinearB Labs revealed developers wait on average 4 days for a pull request review. That’s a lot of idle time. Even worse, we found that the majority of those reviews resulted in an “LGTM” type comment.
Let’s take a look at what your team needs in their code review checklist, what they don’t, and how you can automate code review checklists by repo to improve developer experience and engineering efficiency and ship features faster.
What to Add to Your Code Review Checklist
Let’s start with some of the items that are indispensable in a code review checklist.
1. Identify Obvious Bugs
This is the primary function of a code review: Check if the code is working.
Even great engineers write code that has defects. Often, those defects are quite silly: an off-by-one error, a misspelled variable, parameters passed in the wrong order to a method, and so on.
Code reviews offer a great opportunity to catch these defects because, while the author often misses them, a different person with a fresh pair of eyes will spot them more easily.
2. Look for Possible Security Issues
When reviewing code, try to look for possible security issues that could be exploited. Your CI pipeline should make use of plugins and other tools that automatically check for many types of vulnerabilities. But when reviewing code, you might find problems the author was too sleep-deprived to notice—e.g. controller actions without protection against CSRF or SQL queries that concatenate user input and become vulnerable to injections.
Even great developers can have a lapse in attention and make mistakes.
3. Look for “Clever” Code
Code readability is another vital area you should look into when reviewing code. First, remember that readability is, to a certain extent, subjective. Take a look at the following piece of code:
{ showList && <ProductList />}
This is a common idiom in JavaScript/React, and it allows rendering a component conditionally. However, a C# or Java developer might find that unreadable, and prefer something like this:
if (showList) { let return = <ProductList />; }
In this example, I prefer the first snippet, even though I’m mostly a backend engineer myself. But this is just my opinion. The point is that code readability is a subjective judgment call that must take into account different coding styles and what’s idiomatic for each language.
PRO TIP: When evaluating readability keep an eye out for code that tries too hard to be clever.
What you should do when evaluating readability is to keep an eye out for code that tries too hard to be clever. That includes code that puts brevity above clarity, using somewhat obscure language constructs in order to save a few keystrokes.
4. Check for Code Duplication
When reviewing code, you’ll often spot some low-hanging fruit regarding code duplication. For example, maybe the author planned to extract the duplication to a dedicated method but then forgot to do it. Sometimes there’s a nice abstraction just lurking around and the author can find it with the right push. Maybe the functionality already exists elsewhere and can be reused.
5. Check Whether Names Are Descriptive Enough
Naming is one of the hardest things in software engineering, but that doesn’t mean we should give up. When performing a review, look for opportunities to improve the names of variables, constants, class fields and properties, methods, classes, and so on.
PRO TIP: Look for opportunities to improve the names of variables, constants, class fields and properties, methods, classes, and so on.
Sometimes, the code is trying to be clever with the naming by picking fancier names than what’s needed. A good example is creating interfaces to abstract access to the system clock so they can unit test code that makes use of time. Fair enough, but then they name this abstraction “iTimeProvider,” or something like that, when in the real world there’s already an object that provides the time: a clock!
With the help of a patient, creative, and empathetic reviewer, engineers can find names that are creative yet simple and adhere to the language used by the rest of the business.
6. Look for Possible Performance Improvements
Again, you should have automatic checks and production monitoring to detect performance issues. However, that doesn’t mean you shouldn’t look for opportunities to increase performance while doing code reviews. I’m thinking about easy finds like:
- an expensive operation inside a loop
- excessive allocations of objects
- inefficient string concatenations
- inefficient logging
You’ll often spot seemingly simple problems during the review that will go a long way in improving the code and its performance.
7. Check the Presence and Quality of Tests
Automated tests—including but not limited to unit tests—are code and, as such, you should also review them. When reviewing unit tests, check for:
- The presence of tests: Did the author create tests for their change?
- The quality of tests: Do the tests created seem to effectively exercise the system under test? Do they follow agreed-upon best practices?
- Readability: Remember tests are also documentation. They should be simple and easy to understand.
- Naming: Are the tests named according to the team’s convention? Is it easy to understand what they’re about?
8. Explain Your Changes
When you feel that the author of a delta you are reviewing wrote less-than-perfect code because of a lack of knowledge, it’s important to take the time to educate them. One of the major benefits of code reviews is that they spread knowledge throughout the team and help less-experienced developers level up their skills. Although each team member will have their areas of expertise, the team will perform better if everyone has a comprehensive understanding of the codebase.
It can be hard to take the time to explain something, but think of it as a long-term investment that will improve the quality of the code to be reviewed, which will make reviews easier to conduct in the future. Better than catching bugs early is making sure that the bugs don’t appear at all!
Increasing knowledge among the team will also improve the overall quality of the codebase, resulting in reduced development costs. In this way, investing in learning is like preemptively paying down technical debt.
9. Optional: Code Documentation
I added this as optional because I think it really depends on the conventions and rules of each team. I personally am a big fan of Docblocs. That is code documentation such as XML comments in C#, Javadoc for Java, and phpDoc for PHP. By keeping comments in the code itself, this makes updating the documentation much easier and therefore more likely to happen.
During the build process, these comments can be used to generate documentation that is then uploaded to a server where they can be accessed by developers on the team. Such a style of documentation is particularly useful for teams creating software that’s used by third parties, such as libraries. Your mileage may vary, and that’s why I consider this optional.
What to Remove from Your Code Review Checklist
As a bonus, let’s quickly go through some things that, in my opinion, should not be a part of your code review process.
1. Cosmetic Concerns
You shouldn’t be wasting your time fixing lots of cosmetic/aesthetic issues during your code review. These should be taken care of by an automated tool like a linter or static analyzer. You can configure linters to enforce one of the established coding style guides (AirBnB’s is a popular one) or to enforce your company’s style.
What do I mean by cosmetics? Things like:
- whether to use tabs or spaces
- the position of opening brackets in code blocks
- whether to leave a space after if, for, and other keywords
- whether to leave a blank line in certain situations
- snake_case vs camelCase vs PascalCase
2. Testing
In my view, reviewers shouldn’t test changes when performing a code review. When I say test changes, I mean pulling the branch to their machines, executing the application, and manually going to the changes in order to see if everything’s working properly.
If the reviewer feels the need to test a pull request in this way, this means that either the pull request is too large and complex, or the team lacks a proper testing strategy. Or probably both.
3. Anything That Can Be Automated
As a general rule, avoid doing anything in a code review that can be automated. For instance, I just talked about how you should review unit tests when performing reviews. Sure, but that review should not include verifying whether the tests cover all possible branches of the code. That would be too time-consuming anyway, but there’s an automated way to check that: it’s called branch coverage.
Another example would be compliance or adherence to the tech stack agreed upon. There are ways to automate that, and it should be automated.
Another piece of the code review process that should be automated are notifications. Engineers shouldn’t be taking valuable time to ping another about a PR they created 4 days ago. Instead, set Team Goals with LinearB and use our developer workflow bot, WorkerB, to help dev teams course correct when a review has been waiting longer than your agreed-upon timeframe.
Promote your pull requests to merge 10x faster. Get started with our free-forever account today!
Automate Your Code Review Checklist by Repo
What’s even better than automating aspects of your code review checklist to cut down review time? Automating the whole review. Waiting 4 days for a LGTM review isn’t helping your code quality anyways, so why not?
We’ve set out to tackle this problem and help devs get their code merged faster. It boils down to this: every pull request is different. Some PRs are critical areas of your codebase that need more than one reviewer or a review from a subject matter expert. Some are just docs or image changes that could be merged automatically.
With gitStream, you can create a clear set of rules and policies that automates how code gets reviewed and merged in each repo.
- Why do you need a human review on small changes like text corrections, images or css that passed all tests? Apply an auto-approve check to pull requests and speed up your time to merge.
- Want to assign reviews based on workload, review time, or area of expertise? gitStream review automations make sure the right person is assigned so your PR gets picked up faster and merged with higher confidence.
- Still recovering from a major failure and requiring 2 reviewers on every PR? Now you can relax with confidence. Create a review automation that assigns a specific person or requires 2 reviewers to any code changes in the most critical areas of your codebase.
