The Perfect Code Review Process
T his article talks about a fictional scenario that tries to represent a near-ideal code review process. The story revolves around app development for a company and not an open source project. Also, it should be noted that this is an opinionated article of what makes a good code review. There are many different approaches that can make up an excellent code review process and the optimal process will vary depending on a variety of factors.
That being said, enjoy the story 📖
Meet Jimmy. Jimmy is working away on a new feature for his project. Jimmy and his team are using Git as their version control system and he is making his commits on a feature branch that will eventually get merged into the development (dev) branch (the team follows a GitFlow branching model). Before Jimmy can get his feature merged into the dev branch, his code needs to go through a code review.
Why does Jimmy’s code need to be reviewed before merging? Well, all the code in the dev branch should leave the project in a functional state and follow certain project level standards. Some examples of project level standards include adding the appropriate tests, correctly defining variable types, and having the code logic in the appropriate files/functions. Since Jimmy isn’t a perfect programmer (especially after a late night of binge watching the latest season of Black Mirror), his code needs to be checked by his team members.
Jim-bo works with 3 other team members. When Jimmy has completed his feature, he pushes his feature branch to Github and makes a Pull Request (PR) to initiate a code review. Jimmy then adds one or two of his teammates as reviewers to his pull request.
Why doesn’t Jimmy add all of his team members as reviewers? Well the more people that are asked to review the code, the more hours/resources you are dedicating to reviewing code and those hours can probably best be used on other things than having 3 people review the same code.
If the purpose for adding all team members to a PR is to receive the quickest response, this usually doesn’t work out as expected. There is usually one person that is the first to always review a PR and after this happens for many consecutive PRs, everyone expects the same person to review the pull request and then the requests for review end up getting ignored by most. This isn’t so bad for a team of 4, but the problem gets amplifies with larger teams.
If one of Jimmy’s teammates is a “senior” level developer and Jimmy is a “junior” level developer, this shouldn’t prevent Jimmy from being included in the code review process. Everyone should be able to participate in the code review process as it helps the team learn from each other. However, if Jimmy goes to review the PR and doesn’t understand much of what is going on in the code, he should not accept the PR because he “trusts” the work done by his team mate, but rather someone else should review the pull request.
Before Jim-mister adds his colleagues as reviewers to his PR, he needs to write a detailed Pull Request description that addresses the following:
- What has changed between his feature branch and the dev branch
- Checklist of prerequisites (e.g. added documentation, added tests, ready to merge, etc.)
- How to use the feature (a GIF of the feature in action is extremely useful here)
- How the design has changed and how it compares to the design mockups (screenshots comparing the designs to the implementation)
- Additional notes the reviewers should be made aware of
What goes into a pull request is usually highly project-dependant. Often times, you will see projects that have lots of pull requests make use of pull request templates to pre-populate a lot of the required information for the PR description.
Once Jimmy is done submitting his detailed pull request, he waits…but not too long!
A good review process requires that pull requests get addressed as soon as possible in order to prevent the project from being impeded. Ideally, pull requests are reviewed within two hours of their submission.
Think of this for a minute, if a PR is submitted by Jimmy, he will likely tackle some smaller tasks on his plate, such as responding to email or maybe addressing some tasks on the internal projects he is working on. If an hour goes by and nobody has reviewed his PR, Jimmy will have to start working on a new feature for the project. Let’s say that he is 3 hours into working on a new feature and then he finally receives feedback on his PR which requests some changes. Jimmy then switches back to his old feature branch to address the issues on his code review and then leaves a message on the PR saying that his PR is ready to be re-reviewed. Jimmy switches back to the new feature he has started and doesn’t get additional feedback on the PR until mid-day the following day where he is asked for EVEN MORE modifications. As you can see, this process is being drawn out and requires a lot of context switching, which usually hinders productivity.
So ideally, Jimmy’s PR quickly gets reviewed by his colleagues and he receives requests for changes on certain parts of his feature. The feedback Jimmy receives is worded so as to explain what should be changed and more importantly, WHY it should be changed (some comments even include links to external documentation/articles that provide more detailed information on the subject). This is extremely helpful for Jimmy because he can learn from his mistakes and better understand the best practices that should be followed. The language Jimmy’s teammates use in their feedback is friendly and isn’t in any way accusatory or trying to shame Jimmy.
Ok, now that Jimmy-two-shoes has received his feedback, he can now act on it by applying changes to his code. Ideally, Jimmy uses interactive rebasing when making changes so as not to add extra commits to his branch with commit messages such as “Fixed xxx” or “Added more tests”. Interactive rebasing allows previous commits to be edited with additional changes. Here’s an illustration of what Jimmy’s commits should look like after updating his feature branch with the feedback he received on his pull request:
Notice how, for the recommended way of doing things, the commits are the same before and after the changes have been made to the feature branch? This is because an interactive rebase was used to edit individual commits. So instead of making a commit such as “Added more Jest tests”, the commit called “Added Jest tests” would be modified to include the added tests. This way, all the commit messages are still relevant and then there are not a bunch of extra commits spread about the feature branch.
Ok, so Jimmy made his changes, he pushed his changes to Github and has notified the other reviewers of his changes. If there are additional changes requested by Jimmy’s teammates, he will address those, otherwise Jimmy can merge his changes into the development branch. Jimmy proceeds to high-five all his teammates, bust out some office break-dance moves, and can continue living a full life.
That’s what I would describe as an ideal code review process. Does this always end up being followed? Unfortunately, no. Sometimes reviewers are too busy and don’t make the time to promptly review PRs. Other times the details of a PR are not clearly outlined in the Pull Request message and this makes it more difficult for the reviewers to provide a beneficial review. That being said, it is something to strive for and will make the development process more enjoyable.
Be like Jimmy and his perfect teammates and make the development world a better place for everyone 👫👬👭.
Other resources related to code reviews and pull requests
Giving and Receiving Great Code Reviews
10 Principles of a Good Code Review
