By Lou Bichard July 23, 2018

Why Code Reviews Hurt Your Code Quality and Team Productivity

Recently, I conducted an experiment with my team.

This experiment was challenging enough to make even some seasoned developers sh*t their pants.

What experiment did we run? We stopped branching our code, we threw away code review, and we started pushing directly to master.

If at this point you need to take a bathroom trip, please do.

If you’re a developer with any experience, then there’s a good chance that the thought of this experiment makes you uncomfortable.

I know this because code review and branches are very widely accepted methods for writing great code; in fact, they’re pretty much a sacred cow. Working for many big companies in London, I see the large majority of teams employing standard code review and branching paradigms.

So why on earth would we get rid of these practices? Are we crazy?

But, here’s the thing (our results might surprise you):

Don’t believe me? Let’s look at the numbers … We implemented Trunk-Based Development (TBD) around mid/late April this year.

The following are the reports from our Code Climate instance:

Trunk Based Development

You can see that technical debt was rising rapidly before TBD started to tame and reduce complexity.

And it’s clear that code coverage increased significantly.

Commits also exploded! (The subsequent drop is because the codebase was broken down into separate microservices.)

Our productivity, our “flow” into production, went up drastically.

If you’d like to get more productive, and raise your code quality and test coverage, read on, because I’m going to share with you some of our key learnings from implementing TBD.

We’ll also go through the purpose and the benefits of TBD, the difficulties and problems that we faced when implementing it, and how we moved from fear and worry to curiosity and fearless development.

Because—let’s face it—explaining a technique such as TBD can get very theoretical and you’ll likely start wondering how it works for a real team, with real fears, concerns, and biases.

Let’s get to it.

What Is TBD?

TBD is a practice that tackles the difficulties that come with a branching model and integration of code by encouraging multiple developers to work on the same code branch simultaneously, integrating frequently and with small commits.

It’s the direct opposite of a branching-based approach, where developers could work on many different branches and then integrate back into a single trunk branch in intervals (usually weeks or months).

The finer details of TBD in the real world

At the start of the article, I claimed that we threw away code reviews.

Well, now I need to come clean about something: We didn’t actually stop reviewing code; we just do it in a slightly unorthodox way.

Let me start by saying that I love code review. Mainly because I’ve developed on low quality code before, and it’s not fun. Mountains of unmaintained code starts to stack up and up until you’re swimming in code that is wildly inconsistent and overly complex. These codebases feel like playing Jenga—every change you make could make the whole thing fall apart!

But code review is said to (at least in theory) lead to enriching conversations about code quality and increased ownership. But what I often see in practice is not this utopia.

Instead, I often see reactive behavior being carried out by frustrated developers who bicker about whether a change is necessary for the implemented feature. Usually, the loudest developer wins, while the others roll their eyes and submit to their every whim just to end the pain quicker.

So that leads us to the question: What does the code review process look like in a TBD world?

Rather than wait until after all the code is written to raise a pull request and ask for a review, we pair program. This method effectively makes the post-review process “redundant,” because the code has already been seen by potentially many developers.

Now, I know for some developers this would make them squeamish. Because they know working in pairs might not be enough eyeballs on code that’s written—sometimes we need two, three, or more developers to review code.

But rather than reviewing code with many developers, we cycle pairs frequently. This means that potentially many pairs end up working on the same feature. So rather than simply commenting about improvements and hoping they’re implemented, the pair can actually go ahead and make the changes.

But pairing isn’t the only way we review …

We also review post-check-in. When code is pushed, other developers are notified of the commit and can read and comment. While this might sound quite reactive when you’re making very small commits, they’re easy to revert if necessary or to open up a conversation on how to make an improvement. Because the post commit doesn’t slow down the individual, this leads to far less frustration.

But this unorthodox way of working won’t work for just any team … For some teams, ripping out code review would lead to anarchy. It could get very easy for small bits of bad code to start adding up over time and never get addressed.

So how do we tackle this?

Simply put: Automation.

For instance, we can measure complexity of the code and the code coverage, run static analysis, ensure that commits are in the correct format and that no logs or code comments are left in the code or that any packages were installed with vulnerabilities, and on and on.

You might be starting to see that implementing TBD doesn’t mean just committing to the same branch; it’s a whole different way of thinking about code quality and how we work.

However, apart from sounding like good fun, we do need to think about what TBD really brings to the table and why we’d want to use it …

Why TBD Is Important

At this stage, you know that TBD helps to integrate into a central branch on a frequent basis. You also know that it helps to do pair programming and to heavily automate instead of doing clunky, pull-request-style review. But we will need to cover not just how we do it, but also how it makes us more productive.

As the experiment went on, I was genuinely surprised to see the benefits emerging, and they weren’t what I expected …

Advantage 1. Less work in progress, get more things done

Traditional pull-request-based code reviews slow teams down. To see why, it’s best to look at an example:

You raise a pull request, ask for a review, and then wait several hours. In the meantime, maybe you pick up a new ticket and start to get your head around it when you get pinged with comments on your previous code. So you drop what you’re doing to make the changes, push them, wait a few more hours, get some more comments, and repeat, repeat, repeat.

Sound familiar?

A lot of teams operate this way. But it’s slow.

Pull-request-based code reviews seem to encourage developers to work alone, picking up new tickets rather than working together to get the current work done.

Advantage 2. Better communication

When working on a single branch, it’s important that it’s fully functional at all times. That means if there are any known bugs or defects in the branch, they should be fixed or reverted immediately. But this takes the discipline of the entire team.

If the main build breaks, the team must dive on the build to figure it out immediately, pulling in the relevant developers with the right knowledge.

In a traditional feature-branching model, you might not know that anything is broken until you raise a pull request potentially weeks after you’ve started the code—by which time the code might have changed significantly and require lots of rework.

By having this central build that is sacred, the team started communicating far more than I’d ever seen in the past. Soon, a real sense of ownership emerged to ensure the build was green at all times. Because everyone needs to build to be green, everyone chips in equally.

Advantage 3. Greater confidence in refactoring

When working in pairs, most people get to know the codebase pretty well, meaning there’s quite a high likelihood that the code the pair is viewing has been seen before by one of the developers.

This makes coding far more productive. When a developer knows a code’s structure, it makes finding the right code far quicker. Not only this, but it also gives great confidence in making changes to the code, as the wider impacts of the change are more readily known.

Over the long run, this constant tidy-up ensures the codebase stays clean.

Lessons Learned From Implementing TBD

So we’ve talked about what TBD is, how you could implement it, and some of the benefits. Hopefully, now you’re curious enough to want to see how it could work for your team. Well, if you’ve made it this far, I’m going to assume so!

The inner cynic in me is wary of practices that are considered best in every scenario. It’s rarely that black and white, right? Statements like “always do this, never do this” can seem too reductionist (and if we’re honest, patronizing too).

So let’s talk about the details—the lessons learned—of implementing TBD. I’m sure you want to hear about the truth and the real pains that we suffered getting this whole thing to work.

It wasn’t a straightforward journey, that’s for sure!

Here are six quick-fire things that we learned about TBD.

These should help you to implement the practices and achieve better productivity by avoiding the pitfalls. First and foremost …

1. TBD might not work for everyone (but the reason isn’t what you’re thinking)

Just saying TBD might not work for everyone hurts me on the inside. I really wish it would work for every team; I think all teams could benefit greatly from this type of development pattern.

But … I know it simply wouldn’t work.

Why? Because some teams don’t have the appropriate drive and curiosity that implementing TBD requires. To completely change the way a team has worked for their entire career can be incredibly challenging.

2. The team needs to self-police

Building on why it might not work for all teams is that teams also need to be able to “self-police.”

The teams need to enforce the rules themselves because they believe in them, not because they’re being told to do it. Without self-policing, it would be difficult to roll out a full shift to TBD.

3. Rely heavily on automation

With TBD, you need to think about code quality differently than you might have done before.

Rather than thinking of code review as the only time to implement feedback loops, you can and should choose to do them through automation instead.

Always ask: “What automation would I need to add to give us more confidence that our code is well-written and functional?”

4. Pair program on everything

Failure to pair program will mean you likely need to re-introduce branching to get a review on your code, and this is a slippery slope, so avoid making changes with single developers as much as possible.

Oh, and while we’re discussing pairing … make the effort to swap pairs constantly. Doing so ensures knowledge is spread about and that no code silos emerge.

5. Decouple commits and stories

With TBD, you have to take another mindset shift, and that’s to think about each commit as adding value on its own—which is very different than how a lot of developers think. Most developers are used to not committing their code until a feature is finished completely.

While I’m not saying you should cut corners, I am saying that changes should be brought back into the trunk branch as often as possible. This will feel uncomfortable at first, but push yourself to write even smaller commits than you think is possible.

6. Decouple TBD as a concept from tooling and automation

TBD is at its core more of a cultural problem than a technical one.

In order to exemplify this idea, rather than invest in elaborate tooling and risk losing the essence of TBD, consider adopting the practice without any expensive or time-consuming automation.

For my last team, we used a plastic ornament flamingo. The flamingo was called a “token” … a physical representation of TBD. With the flamingo in place, a team member requests to merge into trunk by taking the flamingo. With the flamingo watching over them, they pull down any remote changes, run all the tests, and if green, they release the flamingo and go about the rest of their day.

This means we can implement TBD without the need for elaborate tooling and without a lot of upfront costs.

It’s your turn now

At first, with TBD I was skeptical. And maybe you are too.

I’ve been a big fan of code review and the security it’s given me as a developer.

But sometimes, more bureaucracy isn’t the answer. Not if it’s productivity that we seek.

Instead, we can choose to empower the team to make small changes, and to encourage the team to talk more often and share their work frequently.

I have one last thing to share:

Do what the Japanese would call genchi genbutsu … which means “go and see for yourself.”

The only way to work out if TBD is for you is by trying it, going in with curiosity, and seeing what benefits you can reap. So I dare you … drop the code reviews, forget the branches, and push directly to master (with tests!).

And see if it works as well for you as it did for us!

About the author

    Lou Bichard

    Lou is a Frontend software developer based in London. He writes on personal and career growth out of www.thedevcoach.co.uk and is the founder of software developer recruitment company www.hacktopia.io.