Code review and keeping in the flow

By: on May 22, 2020

It’s now table stakes for team software development to be done on feature branches and reviewed using a tool such as github’s pull requests. That’s despite, in many situations, a lot of delay while unreviewed code piles up.

If you don’t use feature branches on a project, people are puzzled and get uncomfortable. On a recent project, one repository was tricky to develop with feature branches since it contained terraform code that could only be tested in a shared virtual private cloud. It was only feasible to run integration tests in a single environment, which the developers had to share. I chose to use an alternative model where everyone pushes and pulls work frequently to a shared branch and review happens later. Along the way, we had a number of difficult conversations with people joining the project to explain why I’d choosen to go against the norm.

If you’re not familiar with the feature branch and pull request model, both Github and Atlassian have good write-ups. Other people have noted the problems with feature branches and late feedback, and difficulties with continuous deployment.

There are a number of good reasons that pull requests have become so popular:

You don’t want mistakes from your coworkers slowing you down

If you have more than a handful of developers working on the same system, and if one of them makes a mistake, you don’t want that slowing down the rest of the team. If someone introduces a subtle problem, you might well find you wreck the productivity of a lot of people for maybe a few hours while they get distracted, many fix the same problem and then get back into the flow of what they were doing before. This can be a serious issue, and the impact is proportional to the number of people actively working on a region of code.

If you can keep a working version of each module available to your team while you go in and make changes, then you minimize this effect. Here are a few ways to achieve this:

  1. Use feature toggles so that experimental new behavior is only activated for the one or two people working on that the new behavior, and, most of the time, everyone else gets a stable system.
  2. Do internal releases of the software so that other people are using the last stable version.
  3. Continuous deployment with zero downtime rolling upgrade, which blocks the upgrade or automatically rolls back to the previous version if problems are found.

You often have lots of simultaneous changes being developed

On a recent project it was not unusual to have 30 pull requests open for review, and 50+ feature branches open. We considered it vital to keep the integration branch stable so, each time a pull request was accepted, we’d retest everything, which took about 15 minutes. Overall, it took days or sometimes weeks to get pull requests merged. We achieved our goals of rarely wasting team time on unstable changes, and we could generally make a hotfix to production whenever we wanted.

Each developer found on their first few days on the project that it took hours or sometimes days to get new work into the integration branch. While it is possible for an expert developer to confidently continue working on a feature branch while a pull request is discussed using interactive git rebase or patch queues, I’ve noticed it is just as common for sensible team members to avoid that cognitive load and go find something else to learn or work on while their pull request goes through. In due course, the bottleneck was addressed by evolving to a full microservices implementation with one repository per service. That transition brings a big dose of complexity.

One discipline that helps is for people to start every day reviewing pull requests before starting new work. This is sometimes described as working from ‘right to left’ in the context of a ticket board where the live bugs and pull request columns are at the right. You handle the reviews, then develop committed work, then plan new work. The price you pay is further lengthening the time to get feedback on any work in progress, and often we find that multiple people have stopped to review the same code at once.

You like the pull request review tools

The pull request user experience in github, bitbucket, gitlab, and so on is attractive. It’s easy for people to jump in and make comments and there’s a good audit trail for everything. You can also force your team to follow this workflow; you can mandate that two people have looked at everything. Such technical measures are especially attractive for leaders facing quality problems and losing trust in their teams.

Beware, though, of deceiving yourself into thinking that the fact that someone pressed an approve button somewhere means that they engaged deeply enough to think through the change in detail, or that they understand the system they are looking at well enough to understand every impact of the change. That’s a big burden to put on a reviewer. Perhaps your team culture should be to assign the responsibility for regression with the reviewer rather than the original author, to put the emphasis on detailed and careful review. If you do that, don’t be surprised if it seems to take a long time to get code merged.

You like to teach newcomers via detailed pull request comments

On some projects, coming up with a non-trivial piece of work and then having a senior colleague rip apart the code in detail is a rite of passage. The experience isn’t universally appreciated. It’s true that a lot of good advice can be communicated in code review. Perhaps newcomers would be better getting that advice sooner, particularly if the reviewer is raising a design objection. I find that, no matter what care is taken on design reviews, problems often only appear at the implementation stage. So, even if you produce detailed user stories or 100-page specs with lots of UML diagrams, there can be a big gap from reviewing that design to the next review point if that is a final code review on a branch containing many hours of work.

Often, we hold back on review feedback that feels pedantic. Or, looking at a change can make the reviewer think of a broader problem with the original code, and if we mention that in the branch review then we are potentially forcing people to detour into something which isn’t really what they wanted to do, seizing the agenda away from the pull request author and losing control of prioritization.

You want to use pull requests to stop ‘bad’ code going to production

One good reason for code reviews is that it is often important to make sure that some kinds of inappropriate changes don’t go live. If you are working on a system handling money then this will probably be a compliance rule. More broadly, you may need to consider copyright violations, defamation, or people accidentally checking in huge binary files that clutter up the version control system.

Tools like github let you quickly set up a control to make sure that someone approved a pull request on each change that hits certain branches. Couple this with setting up your release system to only release from such a branch, and it might seem the measures add up to a strong level of protection. The trouble is that there are almost always ways around this—by hacking the build server, changing the build artifacts storage, faking commits to appear to be from a respected colleague, or sneaking a behavior change into a big pile of refactoring.

The collusions around pull requests can get serious. You’ve got people believing, and telling stakeholders, that every change is carefully reviewed. Often the truth is that people don’t like the distraction of code reviews and the developers, recognizing the incentive structure, sooner or later start waving through changes from their favorite coworkers with, if you’re lucky, only a quick glance for something like “Send all the money to Dickon’s offshore account”. The team is painfully aware of how much time they are putting into code reviews, and how much delay the process causes, so it is easy to justify those hours when you are waiting for your colleagues as somehow imbuing a level of quality proportionate to the cost.

Flow and teamwork

If you are a key contributor, the feeling of being able to spend hours or sometimes weeks working away in isolation on a feature branch, taking changes from coworkers only when it is convenient, seems like a huge benefit of the feature branch model. Branches gives control; you can take changes only when you need to, and output seems predictable and high. However, if several people are moving fast and there’s overlap then there will have to be rework. The sooner we find that out, the better. That tranquility and closure you enjoyed while you moved fast in your own private world often turns out to be less productive than you thought at the time.

When team members are struggling, it may be better for others to stop and help them early, rather than wait until they have spent a fortnight going down a path that won’t ultimately get used. I write that being aware of the importance of being in the flow and the cost of unplanned interruptions; the balance is something to continuously assess. If you are at that happy stage in your involvement in a project where you can move fast and gets lots done there’s a good chance that other people are struggling and a few minutes from you might save days of waste.

On most projects, a few key developers get most of the work done. Their pull requests get reviewed and merged as a priority since the team wants to make progress and achieve the team goals. Through comparison, other developers can be demotivated or badly perceived. This creates a vicious circle since often it is quicker to make changes than to understand what others have done. So, the knowledge tends to concentrate in the minds of the key developers, and their output goes up while the team as a whole suffers. Feature branches might enhance the visible output of the key developers, while also making it hard for others to get involved. Two consequences often follow:

  1. Projects often fail after the key developers move on. If you want to create enduring value, think about succession planning.
  2. Typically, feature branches have a single author, and if the key developers are getting most of the work done, they’ll end doing most of the scutwork that could be done by others as a way into the project.

An alternative: pre-ship review

So, considering tthe issues with features branches and code review I’ve described so far, what other options exist? The extreme alternative of eliminating formal code review completely is often an over-rotation and isn’t going to be acceptable in many situations. For one, code review will identify many problems before they affect end-users. And code review is a useful way to communicate. Reviews will happen naturally anyway as people develop the system further, but there’s no reason to assume that this organic review level is optimal.

The half-way house between compulsory feature branches and pull request reviews, and zero review, is to ensure all changes are reviewed before hitting production. If your releases are frequent, then one combined review right before release can work; the standard pull request tools will work for this. If the pre-release reviews start getting large and complex, or if you need to pull in multiple experts, then it is likely to be better to keep on top of reviews as an ongoing process. Tools start to become harder to find.

Henrik Nyh describes how his team create an issue tracker review ticket for each commit to their shared branch. The number of open review tickets can be managed. If people spot something during review, they can either ask the original change author to rework or make a fix immediately themselves in the team shared branch, thus generating a new review ticket while closing one. You’d hope the refinements get smaller as you get closer to release, but the open review tickets metric may not make that clear.

A hypothetical review tool that lets different experts comfortably collaborate to flag peer acceptance of each line of code changed since the last release could be a good alternative. That gives the team a nice, hard metric of unreviewed lines of code which can be managed appropriately. While reviewing contributions people should be free to take the changes a little further themselves and communicate via example rather than criticism. That might make the activation energy required to make small improvements lower, and build relationships rather than upset colleagues with pedantic demands for extra work. This approach handles review coverage in a similar way to test coverage.

Such a line by line shared branch review tool could usefully be integrated with people’s development environments, so that unreviewed code could be jumped to, highlighted and routinely approved in place with a light touch without the overhead of switching to another tool. After all, most of what my colleagues produce, by the time they’ve had the compiler and unit tests work, is fine. When we do need to interact, adding and changing comments could safely be propagated live to the whole team via simultaneous live editing in the manner of Google Docs. My hope is that we can elevate addressing the technical debt to a fast-moving team sport rather than an adversarial situation with lots of back and forward stretching over days—rugby rather than cricket.

Balance

On bigger projects using feature branches, the most precious resource can become integration time. You know this is happening if the number of unapproved feature branches increases over time. That can be a price worth paying for stable integration branches. But we should consider alternatives to the high impact choices we make.

An experienced colleague I worked with often said “if a feature branch lasts more than a few days then just delete it; it probably won’t get merged anyway”. It sounds harsh. People do emotionally invest in their feature branches—I mention it since deleting the branch which is hard to merge is actually sometimes the right thing to do in the long run.

Teams working on shared branches used to be common practice, for lack of tools such as git which make branching and merging easy. I spent years on a system where other people’s changes went straight into my build as soon they checked them in, and people would keep a close eye on what was being checked in, and fix it right away if needed. I still regard that, more than 20 years later, as a high output project. With team discipline and tool support, close working with review coming later can be a strong alternative to feature branches.

Share

Leave a Reply

Your email address will not be published.

You may use these HTML tags and attributes: <a href="" title=""> <abbr title=""> <acronym title=""> <b> <blockquote cite=""> <cite> <code> <del datetime=""> <em> <i> <q cite=""> <s> <strike> <strong>

*