Code reviews are essential for software development in teams. They’re useful to share domain knowledge and best practices within the team, ensure consistent code quality, and lower the number of defects in the software. Reviewing code is hard though.

There are human factors for any feedback to be well received and effective [1]. Are you giving the benefit of the doubt, assuming best intentions? Is the feedback kindly-worded? Is it sufficiently clear?

Challenges surrounding the subject matter are plentiful as well. Have you looked at the change at different scales? On a small scale: Do the changes do what they’re supposed to do? Is this code understandable? How is the naming? Is it tested? With a bigger scope: Do new modules follow established patterns? Is the new behavior in a good place within the project? Are there similar parts that could be extended and reused?

Time and again, the size of the proposed changes has been found to be a significant factor in how hard it is to review code, and how good review feedback is. The bigger a proposed change, the higher the mental load while browsing the diff, and the more time it takes just to read through it all. How do the different pieces fit together? Does this part up here work together with that part down there, and are corner cases not breaking any built-in assumptions? With bigger change requests, the feedback tends to stay at the surface level, since there is a lot to keep track of.

The size of a change has a second effect as well: It increases cycle time. It takes longer until you can put up work for review. More time passes until the reviews come back because it takes longer to review. And it can feel daunting to be confronted with so many changes to approve. There are usually more comments on bigger change requests, and accordingly more iterations.

In this post, I’ve collected a few practices to reduce the size of change requests. They roughly fall into two categories: Preparing so that less change is reviewed at the same time, and splitting up changes after the fact. As a team, you can support each other by splitting up tasks, and sharing the review burden. As a contributor, you can open a scaffolding change request, keep an eye on the change size during development, separate your commits and move them across change requests, or create chains of pull requests.

The descriptions are specific to an agile development process with multiple developers using git as version control system. A change review is the denomination for the code review process unit, usually a patch with a description. Because of the prevalence of Github, Pull Request(PR) is a common synonym. Less common are also changeset, or changelist(CL). Hopefully, the strategies are applicable with minor changes in other environments.

Slicing tasks

The bigger the unit of work, the more needs to change to resolve it satisfactorily. While discussing the work in refinement or planning, you as a team can discuss meaningful intermediary achievements and split up bigger tasks. Reducing their scope reduces the size of their change requests. Splitting up work has some other benefits as well. It may lead to earlier feedback from stakeholders, and increased planning accuracy.

You can also approach colleagues directly, to discuss and split the ticket as you pick it up. Especially in bigger or multi-disciplinary teams, where such discussions might be out of place in an all-team meeting, this is the way to go. Directly approaching a colleague has the added benefit of priming potential reviewers.

There are two dimensions that are common to split along. Rewrite the task from the user’s point of view, finding a simpler form of the feature, maybe with less special cases, and then add follow-up steps that flesh out the basic case. Another typical approach is to imagine how the feature will affect the whole system and create tasks that stop at the application layer or module boundaries.

Examples for the former would be creating the basic functionality without a confirmation dialog and micro-animations, and then adding those in a second and third step. The idiomatic example for the second dimension is a split into backend and frontend work. Although in bigger projects, each of these usually have layers in themselves, so there are more boundaries to split along. Such as fetching and maintaining the data in the frontend, and then showing it as a second step.

With all these newly split tickets, you’ll have to practice a bit more self-restraint. It can be tempting to resolve multiple tickets within one change request, or to drop the follow-up tickets since the basic functionality is already present.

Share the pain

If you feel that your team often faces issues with unwieldy change requests, then sharing the code-review duties spreads the burden. It ensures all team members can read code, and helps foster empathy. In an ideal world, everybody would do code reviews already anyway.

Learning from your fellow contributors’ changes is as important for the benefits of code reviews as is receiving feedback on your own work. Some patterns are more evident when observed in a limited change request, rather than mixed in with all the other parts of the existing codebase. Browsing code in reviews can open your eyes for similarities, and you can learn about parts of the product domain you’re currently not contributing to.

Depending on the experience profile of your team, sharing the review burden might involve some coaching. Even if you lack the confidence to give final approval, try to review and leave feedback, and encourage colleagues to do the same. Questions are an important form of feedback. Code that is hard to grasp is hard to maintain. Additionally, more than one person can review. Even with diminishing returns, every new review provides another view on the codebase and the product domain, and catches defects before they are encountered by users.

Setting aside timeslots throughout the day can help to get into the habit. For flow-reasons, time after context changes offers itself naturally: First thing in the morning, after lunch, or after meetings.

Nothing to see

For larger features or code cleanups, a scaffolding change request can be a good basis for discussion. In it, you prepare the structures while leaving all (or most) of the implementation for later. Indicate with comments what you have planned. This allows reviewers to focus more on strategic feedback and does not trap them in the minutiae of your changes. And then, based on that branch, you can fill out the comments with implementation in subsequent change requests.

In some teams, instead of merging the scaffolding branch and expending effort to keep its features out of production, the scaffolding branch would live as the trunk for all tickets regarding this feature until it is ready for release. The upside is that no dead code will end up in the main branch. The downside is that you now have to maintain long-lived branches. You’re essentially maintaining two similar versions of the same code. In smaller teams which can all share the new feature work, and keep the main branch mostly stable, that downside might be acceptable.

If you do apply the scaffold, since you add code that is currently not functional, you have to hide it in some way on the production systems. Conditional branching based on the environment, or feature flags, are common patterns. What you end up with depends on your problem, your deployment tooling and processes, and the patterns established in your team.

With aspirational code, it is important to keep in mind that code is not a static entity, but grows and changes as we deepen our understanding of the problem domain. Even the best-planned scaffold might include beams that are not holding up any floors, or end up in a doorway. And once we realise that, they have to go.

Step by step

If you implemented a feature and the proverbial horse has left the barn, due to feature’s size or explorative programming, help is harder to come by. Before throwing the changes over the fence and hoping somebody else can make sense of them, try reviewing them yourself. If it’s too long to review yourself, how could you expect somebody else to do it? While reviewing, mentally split the changes into units that each constitute their own change request. Units of change can be modules, but they tend not to be since each unit should make sense as its own addition to the code. It should not introduce too much aspirational – dead – code and leave CI green.

For then actually splitting your changes up, you should lean heavily on your version control system. Tracking changes is its job. As a first step, rewrite your history [1, 2] with a combination of interactive rebases for ordering and squashing the changes, and soft-resets and partial recommits for grouping and splitting up changes. Once you’re happy with the history, you can create branches and (relative) change requests for each logical step. If you get feedback and adjust the first steps, rebase the follow-up change requests as you merge. git rebase --interactive has been a very handy tool for me.

A word of caution: this requires more care with rolling up the ladder of change requests. How to do this best is up to your team processes and in particular how you manage version control history of the main branch. At bitcrowd we continue the established practices when working on existing codebases, but when we can choose, we usually do squash-merges. In that, each of the intermediary change requests becomes one commit on the main branch. ‘Why’ is a topic for another post though. If you forget to change the base of a change request before merging, you might need to cherry-pick the commits, rebase again and re-open the request.

All of these steps may seem like tedious work, that does not provide value to your end goal. But in our experience, they shift a bit of the burden from the reviewer to the contributor, increase the quality of the feedback, are more effective at sharing the domain knowledge, and allow fewer errors to pass through. All of which are reasons why we do reviews in the first place. Or, to put a stronger point on it: If you don’t care enough to get good feedback, you might want to drop that hollow practice altogether.

Drive-bys

Following the scout rule best practice, you can end up with a request that includes multiple changes that are more or less unrelated. Fixing a typo here, improving naming there. Maybe moving some function to a better module. Creating a change request for each of those changes immediately would interrupt the workflow and this additional effort decreases the likelihood that I fix things as I encounter them.

Unfortunately, resolving multiple tangentially-related issues in one change request both increases the amount of code to review, but more importantly, it increases the mental burden on the reviewer who has to find and separate the different threads of change.

In most distributed version control systems, when committing your changes, you can commit all local changes, or just some of them (git add/commit --patch for the latter). Creating a commit per drive-by change is much less effort than opening a change request directly. Later on, you can cherry-pick those drive-by commits onto the main branch and create small change requests that are easy to review and approve. As the drive-by changes are merged, rebase on main interactively (git rebase -i), and drop the drive-by commits from your branch history.

Just like splitting an existing change request, this leans heavily on and requires more familiarity with your version control system.

Mind the diff

Having a look at the diff and diff summary can be a decent indicator whether to wrap up your local changes into a reviewable unit. With experience, you can develop an intuition about when a group of changes you have made is ‘big enough’. And a look at the diff summary usually indicates that about 200 lines were updated or added. Once all the new code is tested and moved to good places – and CI is green – you end up in a +300 range change request, which is definitely on the large side, but usually still manageable. The absolute numbers may vary by technology though.

A big caveat with this practice is that each change request should be a cohesive unit. Wrapping up preliminary changes so that there is little hand waving and aspirational code is itself not a trivial exercise. Splitting tasks in the first place can help with that.

Downsides

Before I close, I have to point out that there are costs involved with reducing change request size.

The complexity of dependent change request chains can lead to user error. Rebasing and waiting for CI is time that you will not spend resolving customer problems. But maybe that is a chance for reviewing, or manual quality assurance.

If you split your code too much, and deal with changing priorities, you might end up with half-finished features in the main branch: aspirational code that is hard to understand, and where it is unclear when the code can be taken out again, because the feature will never come.

Additionally, many of these practices require familiarity and fluid use of your version control system. Which in some ways is an orthogonal ability, because it will not contribute directly to resolving tickets.

In our experience, if you’ve decided to do code reviews, the additional effort to keep them at a manageable size is negligible overall. You might spend more time preparing change requests. But that helps reviewers and you might even get better feedback. A review process that is hard on the reviewer, in turn, can slow down feature work considerably.

Conclusion

I’ve shown a few ways to reduce your change request size. One thread throughout the practices is that the amount of change to resolve a ticket is kept down. Split up the tasks before starting, and lay out the feature architecture before filling in all the details. Another thread is to lean on your version control system to both separate changes into independently reviewable units, and observe changes as you go.

As with most things organizational, reviews are a practice that benefits from exercising. There is no silver bullet.

Happy coding and merry reviewing 🙂.