This post is adapted from a GitLab Unfiltered blog post written by me, David O'Regan. In part one of our series, we explain the importance of fairness and empathetic thinking in code reviews and in part two we explain why patch files bring added value to code reviews.
The GitLab handbook defines iteration as doing the smallest thing possible to get it out as quickly as possible. If there was a single guiding principle I could suggest you lean into with your merge requests it would be iteration. At its heart, software is all about iteration. Software is about taking a large problem and breaking it down into smaller, more manageable problems. Like any other skill, iteration needs to be learned and practiced often to improve. The next time you're hitting the "Submit merge request" button, pause a moment and think if the merge you're about to submit could be be downsized.
Why smaller MRs are better
The only thing worse than writing a long merge request is reviewing a long merge request. This is why at GitLab, iteration (and by extension, small merge requests) is one of our driving values.
We even created a DangerBot that will ask code authors to break down merge requests that are over a certain size.
Massive merge requests can create technical problems for a code reviewer beyond added complexity. If a review goes beyond a certain number of lines, it simply becomes too difficult to reason through without checking out the branch, booting the project, and smoke testing. While smoke testing complex reviews is a great idea, this process shouldn't become a habit for reviewing code. Big MRs can lead to merge conflicts, content rot, and other disasters.
Sarah Yasonik, backend engineer on Monitor at GitLab, suggested that reviewers handle too-large or too-complicated merge requests by creating new, smaller MRs while reviewing, and reviewing the code in chunks. It's better to break up a too-big MR than to continue adding lines of code to an MR that is already too large.
The art of the follow-up
As the code author and code reviewer, there are a few best practices to abide by. Namely, if you are a code author and you offer a follow up review, be sure you always follow through on this promise.
If you are a code reviewer, here are four tips:
- Feel empowered to ask the code author for a follow up
- Accept any offers of a follow up graciously
- Be patient with code authors
- Know when it's best to reject a follow up offer
Practical tips for using iteration in code reviews
Why does iteration matter?
The smaller the merge request, the easier it is for the code reviewer to check. The idea of shipping small changes is consistent with GitLab's iteration value. Clement Ho, my frontend engineering manager who has since left GitLab, was a major champion for iteration. Once I started paying close attention to how Clement broke down merge requests into small bites, I started to notice the benefits of iteration almost immediately. Iteration is so important to GitLab that CEO Sid Sijbrandij hosts weekly office hours devoted to breaking down big projects, and grades our team members on their iteration competency.
How small merge requests helps your reviewer
If iteration is all about releasing the minimal viable change (MVC) in small merge requests, then it follows that engineers who fully embrace iteration will be shipping less code per merge request, to the delight of their reviewer.
We've all been there. We are assigned as a reviewer on an MR, and just as you're about to get comfortable you open the MR to see more than 1000 lines of code across multiple files. Time to refill your mug of coffee and get ready for a tiring review process.
The problems with large MRs should be obvious if you've ever practiced self-reviews or found yourself in this situation. Here are a few reasons why large MRs are indicative of bigger problems:
- Longer MRs have more lines of code
- There is the greater chance for brittle connections
- It becomes harder to follow the path of the solution/feature
- Screenshots usually cannot account for the volume of change
- It's much easier to miss bugs
- The author is sure to be left with lots of comments, which can be demoralizing
It's a simple concept, but one that is undervalued. Keep your merge requests small because:
- There are less lines of code to read
- Different contexts are separated into individual MRs
- The reviewer can follow along more easily
- It's easier to follow the path of a feature's development
- Less reviewer comments per MR is better for motivating the code author
In the end, we review code carefully at GitLab because we want to ensure that every release brings new value to our customers. If you have questions or comments about code reviews, creating smaller MRs, or iteration, leave us a comment on this blog post!
Get more code review tips by reading the other blog posts in our series. In part one, we discuss the role of fairness in code review and in part two we share some practical advice on using patch files.
Sara Kassabian contributed to this blog post.
Cover image by Jon Sailer on Unsplash
