It has become common knowledge that Code Reviews are an important aspect of building an effective team. Many articles have been written on this topic, such as this paper, An Empirical Study of the Impact of Modern Code Review Practices on Software Quality. In reality, countless teams in many organizations have done some form of code reviews.
The reality is that when code reviews start, people are excited, and then code reviews become more of a formality, either unclear or difficult to implement. In the long run, this deprives teams of opportunities to accelerate learning, share knowledge, and ultimately improve the quality of their code.
At Shopify, we are not only focused on the long term, but also want to grow faster. In our experience, good code Reviews practices have a huge impact on the growth of engineers and the quality of the products we build.
Nightmare coding experience
This scenario is familiar to many of you:
You’ve just joined a new team and your leader has quickly assigned you a coding task. As a newcomer, you want to express yourself because you want to show off your coding skills. So here’s what you do:
- You code frantically for three weeks to get the job done;
- You submit a Pull Request for review that contains about 1000 lines of new code;
- You get two comments about the code style and a question about the reviewer saying he didn’t understand the purpose of the code;
- You fix the code style and answer the reviewers’ questions, and then the reviewers approve the code you write;
- You branch code into Master, eyes closed, fists clenched, teeth clenched, waiting for the results. A few minutes later, the CI is complete. Fortunately, the Master didn’t crash. However…
- For the next six months, you live in fear, not knowing when and how the code will crash.
You may have experienced the above nightmare, so let’s talk about how to improve the process.
Practical Code Review practices
At Shopify, we value speed of delivery, learning, and long-term growth. These values, while sometimes at odds with each other, have led us to experiment with many new technologies and drive team change.
In this article, I’ve summarized a series of practical techniques used internally by Shopify. With these techniques, we can deliver valuable code that will stand the test of time.
Term description: We defined Pull Requests (PR) as a unit of work to do code reviews before merging into the base branch. Github and Bitbucket users will be familiar with the term.
Breaking a Pull Request into smaller code segments is simple and can be one of the most useful techniques for improving the Code Reviews workflow. It works for two main reasons:
- It is psychologically easier for reviewers to start and finish reviewing a small piece of code. Greater PR naturally leads reviewers to delay and delay reviews, and to a greater likelihood of being interrupted during reviews.
- As a reviewer, if the PR is too long, it’s hard to dive in. The more code we examine, the more brainpower we need to understand the entire block.
Breaking PR into smaller pieces of code gives you more opportunities to get deeper reviews in less time.
Currently, there is no universal standard that applies to all programming languages and all types of work. For internal data engineering projects, our principle is to keep PR to 200-300 lines of code. If this threshold is exceeded, we generally break it up into smaller pieces.
Of course, we should also be careful not to break the PR down too small, as this means that reviewers may need to check several PR’s to understand the overall logic.
Use the Draft PRs
Have you heard the analogy between building a car and drawing a car? The metaphor goes like this:
- The user wants you to build a car;
- Six months later, you build a beautiful Porsche;
- After you show it to people, they ask if it’s big enough for their five kids and their surfboard.
Clearly, the problem here is that the goal is not clear, and the team builds a solution without gathering enough feedback. If after the first step, we draw a sketch of the car and show it to the user, they will ask the same question, so that we can learn more about the customer’s needs. That saved us six months of work. Software is no exception, and we can make the same mistake of putting a lot of work into features or modules that users don’t need.
At Shopify, Work In Progress (WIP) PRs is typically used to get early feedback, with the goal of validating direction (algorithm, design, API, and so on). Change early to avoid wasting energy on details, polish, documentation, and so on.
As a coder, this means being open to changing directions.
At Shopify, we believe in the principle of allowing people to have their own understanding without being opinionated. We want people to be confident in making decisions based on good reasons, but also open to learning about new and better alternatives. In practice, we use Github’s Draft PRs, which make it clear that the work is still moving through the process and Github does not allow you to merge a Draft PR. Other tools may have similar capabilities, or at least you can add a WIP tag to your normal PR to make it clear that the work is in the early stages. This will help your reviewers focus on the right areas and provide the right feedback.
One PR Per Concern
In addition to the number of rows, another dimension to consider is the number of problems your unit of work is trying to solve. A concern can be a feature, a bug fix, a dependency upgrade, an API change, and so on. Did you introduce a new feature along with the refactoring? Fixed two bugs at once? Introducing library upgrades and new services at the same time?
- Breaking PR down into individual concerns has the following effects:
- More independent review units, which means better review quality;
- Fewer people are affected and can therefore be clustered in fewer areas of expertise;
Atomic rollback, which can roll back small COMMIT or PR. This is valuable because if something goes wrong, it is easier to determine where the error was introduced and which parts are rolled back.
Separate the easy from the hard. Suppose you have a new feature and need to refactor a frequently used API. You can change the API, upgrade the dozen or so calling sites, and then implement this feature. 80% of your changes are not functional changes that can obviously be ignored, while 20% are new code that requires careful attention to test coverage, expected behavior, error handling, and so on, and may undergo multiple revisions. For each revision, reviewers need to browse through all the revisions to find relevant sections. By splitting it into two PR’s, it was easy to get most of the work done quickly and optimize the review effort to focus on the hard ones.
If you end up with a PR that contains multiple concerns, you can break it up into separate pieces. This allows for separate reviews for each piece, with faster iterations for each review, speeding up the overall PR review cycle. In general, some of the work should be done quickly before the code becomes unusable and causes merge conflicts.
Decompose PR into separate concerns
The PR of the previous example contains three different concerns, which we split. As you can see, there is much less context for each reviewer to examine. Most importantly, once any part of the review is complete, the code author can work on the issues that have been reported while waiting for feedback from other reviews. In the most extreme cases, the code author receives review feedback from various sections and can work through the PR series almost continuously, rather than finishing the first draft, waiting a few days (already busy doing other things) and then finally coming back to process the feedback.
Focus on the code, not the people
Focus on the code, not the people, and this practice talks about how people communicate and relate to each other. Basically, this is an advocate that we try to focus on how we can improve the product and avoid the author taking the review as a personal criticism.
Here are some tips you can follow:
- Reviewers can think, “This is our code, how can we improve it?”
- Give a positive opinion! If you see a good part of the code, add a comment and praise it. This keeps the author on the good side and helps him to be psychologically receptive to suggestions for improvement.
- Think of it this way: reviewers have good intentions and don’t take comments personally.
- The following table lists some of the review feedback that fell short and how to rewrite it as suggested above.
In the final analysis, Code Review provides us with an opportunity to learn and learn from each other, and we should be open and welcome to it.
Select the right judges
- Deciding who will review your work is often challenging. The following questions are for reference:
- Who has the context of the feature or component you are building?
- Who knows the language, framework, or tool you’re using?
- Who knows enough about the subject to make sense of it?
- Who cares about the results of the code you write?
- Who should learn these things? Or, if you’re a rookie programmer reviewing “old birds,” take the opportunity to ask questions and learn. Don’t be afraid your question is too naive, a strong team will find time to share knowledge.
No matter what principles your team follows, remember that as a code author, it is your responsibility to seek and receive high-quality code reviews of your code from the appropriate people.
Provide critical context to reviewers
Last but not least, your PR description is critical. Depending on who you choose to judge, different people will have different contexts. It is the responsibility of the author of the code to provide links to key information or more context to help the reviewer provide valuable feedback.
- You can include the following questions in your PR template:
- Why is this PR necessary?
- Who benefits from this?
- What could possibly go wrong?
- Have you considered any other options? Why did you decide to take this approach?
- How does this affect other systems?
Good code is not only error-free, but also very useful. As a code author, make sure your PR description ties the code to the team’s goals, preferably to the feature or defect description in the backlog. As a reviewer, you will review the PR description first, and if it is incomplete, you will not be able to determine if the code is appropriate for undefined goals, so it is better to type it back before reviewing the code. Remember, sometimes the best result of a code review is to realize that the code is not needed at all!
What will we learn?
By adopting some of the techniques above, you can greatly affect the speed and quality of the software build process. But beyond that, there are potential cultural implications:
- The team will reach a consensus. The team will know more about your work, and other team members besides you can improve this part of the code base.
- The team will share responsibility. If something goes wrong, not just one person’s code needs to be fixed, but the entire team’s code needs to be fixed.
Any team member should be able to take a few days off without putting the company at risk or constantly checking email in fear of the end of the world.
How can an individual improve the team’s code review process?
If you’re a team leader, start experimenting with these techniques and figure out what works for your team.
If you are an independent contributor, discuss with your supervisor why you think code review technology is important and how it can improve efficiency and help the team.
Talk about it in your next one-on-one or team meeting.
Reference reading:
Engineering.shopify.com/blogs/engin…
The author | Alejandro Lujan planning | winter rain; Edit | vanguard