I’m writing this blog post as part of my official capacity as a developer at Mobiquity to set code review standards in general and specifically for web / hybrid development. Code reviews are a task that I have done for many years, and they’re actually something that I enjoy quite a bit. No one doubts the merits of code reviews or their necessity as part of an SDLC, but I think many developers seek guidance when it comes to performing them. Many are not sure whether or not they have a responsibility to do them (they all do). The purpose of this blog post will be to provide some amount of this guidance. I will discuss:
- What code reviews are, and their goals
- Standards for pull requests and code reviews
- Tips for reviewing web / hybrid development specifically
- Frequently asked questions about code reviews
I’ll make reference to these throughout the post, so I want to define them here.
- Reviewer: The person performing the code review. More than one person can and in many cases should participate in a code review as a reviewer, but I will only use the singular form for the purpose of this writing.
- Pull request: When a developer has completed some code and they want to release it upstream, they will generally open a pull request to do so. This can be seen as a request for a code review: another developer (reviewer) will check the code and merge it upstream as appropriate. It’s very often that this is done through GitHub pull requests, but that’s certainly not the only way to make such a request. There may be many other formal terms for this process, but I’ll refer to it as a pull request and will generally talk about pull requests from the perspective of GitHub.
- Author: The original developer who wrote the code and created the pull request. There may be more than one of these as well, but I’ll just say “author.”
What Code Reviews Are
Before discussing what code reviews are, I’d like to talk a bit about what they are not: code reviews are not a critique of a developer’s code. This is not to say that code can’t be critiqued or judged, but this is not the purpose of a code review. Negative or critical comments are generally not welcome during a code review, and they can actually do more harm than good since they may lengthen the process by making the author defensive. They may be less open to making changes or less open to communication with the reviewer to work together to resolve issues more quickly.
I see code reviews as an active discussion between a reviewer and an author to achieve certain goals in contemplation of merging the author’s code upstream. A code review is a discussion in that there is no expectation that a reviewer will be totally right and an author will be totally wrong. The two need to work together to come to an agreement about code changes. The reviewer may have good points, the author may have good points, and they should ultimately be able to agree on the changes that are going to be merged. That is to say: a code review is the responsibility of both the reviewer and the author.
It is also active in that a reviewer and an author shouldn’t be passing information back and forth asynchronously. Instead, they should work together simultaneously to complete the code review. Code reviews should be considered part of the actual feature request development. Developers have a tendency to work on a feature, open a pull request, and leave for the day — possibly with the expectation that a reviewer will take a quick look after hours and approve it overnight. If there are any issues with the code review, the code can’t be merged in and the feature won’t be completed. Developers are not done with a feature when they get to a stopping point in their code. They are done when their code is merged into the upstream branch, and that requires completion of a code review.
The actual practical process of a code review is difficult to define. What will typically happen is that a developer will open a pull request and let other developers know about it — possibly by assigning it to someone. When the reviewer gets a good opportunity, they’ll make a first pass and leave initial comments suggesting updates and asking for clarifications. At this time, I think it’s good for an author and reviewer to start working together synchronously to resolve the issues rather than going back and forth through GitHub. That being said, I don’t think the act of a code review needs to follow a specific process. I prefer the use of a checklist that you’ll read later.
The Goals of a Code Review
Code reviews have several active goals:
- To catch issues or mistakes, and to confirm the code completes a feature or fixes a bug.
- To ensure that the code meets standards.
- To provide a learning opportunity for authors and reviewers.
- To give another developer on the project (the reviewer) an opportunity to understand functionality that they did not work on themselves so they can update or debug it more easily later.
The first goal is probably the one that no one would dispute, and I think that people may not even consider the last two points to be goals at all. However, I think all four of these are critical to completing a code review. Certainly if there are any mistakes in the pull request they should be pointed out and fixed. If the reviewer has an idea for a change that will make the code cleaner or more efficient, they should present that as well.
A reviewer still has to remember that a code review is a discussion, and if they offer a different way of doing something it doesn’t necessarily mean that the way the author did it is incorrect or bad. In fact, it’s a good opportunity for the reviewer to see the author’s point of view and understand why they implemented something the way they did. Perhaps it was easier for them or they didn’t know about a better way, or maybe they had to do it the way they did it because of some quirk in the software. Either way, it’s good that the reviewer and author both understand why or why not something should be done, know about different options, and be able to agree (or agree to disagree) about how something should be implemented.
Just remember that as a reviewer you need to be able to justify your request for changes. If you think the author should use a
forEach method instead of a
for loop, you need to be able to explain why. Personal preference is not necessarily a great argument. Consistency with the code for the rest of the app is a good argument. In this way, code reviews are a learning opportunity for reviewers since it presents them with a challenge of being able to justify how they write their code — whether they agree with what the author has done or not.
Authors need to keep an open mind about the reviewer’s comments as well. Just as the reviewer needs to be able to justify his request for changes, the author needs to be able to justify how they wrote the code in the first place.
Authors and reviewers should consider each other to be a team. They both have the shared goal of making the code better and learning from each other.
The goal of learning from each other also has nothing to do with seniority. There is an expectation that a more senior developer will be the reviewer of a more junior developer’s code, but senior developers still need their code to be reviewed just the same. The expectation is not that a more junior developer will perform a review at the same level as a senior developer, but the process and goals to strive for should be the same regardless of the level of the author and the reviewer. A junior reviewer should never go into a review with the expectation that
Well, this code was written by a more experienced developer, so I won’t catch any issues or be able to offer any suggestions.
Quite the opposite — junior developers should jump at the chance to impart knowledge to someone more senior. A true senior developer will love to learn something new and to fix mistakes that have been caught in their code.
I think the final goal: To give another developer on the project (the reviewer) an opportunity to understand functionality that they did not work on themselves, is actually the most important and should be the criteria for merging. If you as a reviewer do not understand the code from the pull request in a way that you would be able to write the same functionality from scratch, the code is not ready to be merged.
The expectation is not that the reviewer would be able to memorize and exactly rewrite the functionality the same way the author did. The goal should be that if they had to, the reviewer would be able to implement the same functionality if the author’s original changes were somehow undone. They understand the nuances and gotchas of the code and why certain choices were made about how to do things.
In the same vein, a reviewer should be able to understand the code enough so that they could debug it or update it later … because it’s very likely they will have to do so eventually.
Standards for Pull Requests and Code Reviews
The first step to a code review is to review the metadata of the pull request. If this isn’t up to snuff, then it means that the code itself is not ready to review either.
Checklist for Pull Requests
Writing a good pull request is as important as writing good code. This takes a bit of time and effort, but it makes things a lot easier on the reviewer which will ultimately make things better for the author.
My least favorite kind of pull request is something like:
There’s no useful information here. If a reviewer wants to know what was actually supposed to change in this ticket, they would have to go see what TCMP-2913 is on their own. The PR name is automatically generated from the branch name, and I think this signifies laziness. It’s an ugly name with automatic title-casing and stripping out of some characters. The author didn’t take the time to give the PR a good title to set the tone for the review.
A good pull request has the following features:
- An associated JIRA ticket (if applicable) should precede the title of the pull request. If this is user story functionality, the ticket number for the user story itself is fine. Occasionally more than one ticket number may be appropriate. If you’re not using JIRA, you should include relevant information that ties the pull request back to your issue tracking software.
- The title is a short, <100 character description of what was done.
- The content of the pull request lists all JIRA tickets completed. If you were working on a user story with 3 tasks and you completed them all, you would list all three tasks here. I often fix multiple bugs in the same pull request — especially if they are related. I list each bug number.
- The content also includes a description of what was changed and implemented. Authors should note anything of interest in the code they reviewers especially need to pay attention to or that they are unsure about. Authors should feel free to provide any information they think will help the reviewer. Reviewers should not feel like they have to refer to the associated tickets to understand what’s going on.
- An animated gif, screenshot, or video demonstrating the functionality that was created or fixed should be included. https://www.cockos.com/licecap/ is a good tool for recording gifs. Even if you are not making visual changes it might still be appropriate to include some kind of media such as a screenshot of an updated request/response. If an author chooses not to include visual media in their pull request, they should have an explanation as to why. I think it’s fine to omit them in some cases, but taking the time to record functionality also gives you another chance to test it.
I’ve taken up a new habit of reviewing my own pull requests as well. I’ll leave comments about pieces of the code that I think may need some explanation. This also provides me an opportunity to see whether I should actually be commenting in the code rather than just explaining in a pull request. Authors should always review their own code.
There are also a few things an author can do while they’re implementing their changes to make things easier on reviewers.
- Keep pull requests as small as possible. Try to do changes in small, logical chunks. The more code you have, the harder it is to reason about. If you have 100 lines of code vs. 50 lines of code, it puts more than double the strain on the reviewer to understand the code holistically. The more changes done at once, the more prone to introducing issues or mistakes.
- Keep commits small as well. If you have to have a very large pull request, it will be helpful if you can at least break the pull request down into smaller commits that be digested more easily on their own.
- Large chunks of formatting changes should be done in separate commits. It’s not unusual to have a commit like
Fix linter issues. Reviewers can more or less safely ignore such commits since they have no effect on functionality even though they may be a large part of the diff.
- Review your own changes first. Treat them as if you didn’t write them. See the checklist for code reviews below.
- After you’ve made your last commit / rebased, but before you open your pull request, make sure the code builds, tests pass, and there are no linter issues. Even if you make one tiny change you should still do this.
This rule may only apply to me, but any time I’ve said, “I’ll just make this one small change. I don’t need to test it,” it breaks everything in some catastrophic way. Always test your changes no matter how small. Of course, any time I make a small change and do test it, it works perfectly.
Checklist for Code Reviews
Reviewers should use their judgment about what should be included in their review. Nevertheless, there are some key questions that should be answered before considering a code review to be complete.
- Does the pull request complete the pull request checklist?
- Does the code build? This is more of a spot-check. I’ll go over whether reviewers should be building the code in the questions section below.
- Does the code meet formatting standards? Is it consistent with the rest of the code in the app?
- Does it achieve the desired functionality or fix the desired bug?
- Are any quirks or oddities properly commented? Are comments in general used appropriately?
- Is any version controlled documentation updated appropriately?
- Are any configurable or secret properties excluded from version control?
- Does the reviewer understand the concepts of the code in a way that they would be able to recreate its functionality?
Web / Hybrid Code Reviews
I’ll do this section later. I want to get the other information out there, and I need to take some time to put something really good together for this.
Frequently Asked Questions
These are questions that I have personally received or read about with respect to code reviews.
- How long should code reviews take?
This will obviously vary depending upon the size and complexity of the pull request. At some point I think I read something along the lines of code should take three times as long to review as it did to write. While it would be nice to be able to be this thorough, unfortunately we just generally don’t have the time. I wouldn’t put a specific time limit on code reviews since it depends on the changes, the author, and the reviewer. As long as you’re completing the code review checklist don’t worry too much about how long it takes.
- Should reviewers pull and test the author’s changes?
This is a point of contention between reviewers. My stance is that generally reviewers don’t need to test the author’s changes. My reasoning for this is twofold:
- Authors should be testing their own code before reviewing.
- We have manual QA which obviates the need for a reviewer to test manually.
That being said, I wouldn’t say you shouldn’t pull a branch and test it out. If this is something you feel you need to do to complete the review properly, then I say, go for it.
- Why do we care so much about formatting?
Formatting is also a point of contention both in how it should be done and how important it is. I think we should use tools that are available to us to make sure we have a consistent format when possible — https://eslint.org/, https://palantir.github.io/tslint/, and https://github.com/prettier/prettier are all tools that will help with this.
While we work to set these up and standardize around them, we depend on reviewers to make sure that the code is formatted consistently. I don’t have a strong preference for how code is formatted (as long as it’s internally consistent) except for a few of things:
- Use soft tabs (spaces). This allows indentation to appear consistently regardless of the tool used to view it.
- Try hard to break lines at 80 characters. Going beyond this reduces code readability since the reader’s eyes have to scan over.
- Use a trailing comma on multi-line array, object, or function declarations or calls. There are no downsides to this, but two benefits: 1. If you need to add another line after the last, you don’t have to remember to add the comma. Forgetting this would cause an error. 2. If you do add the comma where it wasn’t before, it increases the size of the diff since there is more than one line of changes.
As for the argument for formatting code consistently, it is actually to reduce distractions to developers who are writing the code. You may think that it will take more time to properly format code (if this is the case, we need better tools), but a developer who inherits your code and is distracted by bad or inconsistent formatting will have to take even more time to format it properly when it could have been correct in the first place. This also creates an unnecessary diff.
Inconsistencies in formatting are also generally distracting. When looking over a file, if you see string literals with both single and double quotes you may instinctively question the inconsistency or wonder if there is a need for a particular quote choice whereas consistent quoting will be unnoticed. Incorrect spacing also makes control flow harder to follow.
Finally, taking the time to make sure your code is properly formatted gives you an opportunity to review it.
Code reviews are a critical and necessary part of any SDLC. I would go as far as to consider them part of the actual feature development or bug fix step itself. The main goal of a code review is to reduce bugs by catching semantic or other errors, but it also serves as an opportunity for reviewers to learn code they didn’t write for the app they’re working on. It also gives authors and reviewers a chance to learn.
Code reviews should not be seen as a senior developer criticizing a junior developer’s changes. Instead, code reviews should be an active discussion between peers of any level about the best way to write code to achieve a task.