Pre-merge Code Reviews

Rules, benefits, patterns, and anti-patterns for reviewing code before merging it.

Published on 26 October 2013 by @mathiasverraes

I’m a big proponent of pre-merge code reviews. From my experience consulting for teams in problematic projects, I can say that (along with daily standup meetings) pre-merge code reviews are one of the most effective and yet fairly easy changes a team can introduce to radically improve the condition of the project. And even if you consider your project to be healthy, there’s always room for improvement.

The Rules

  1. Never Push to Master
    Always push to a separate branch per logical unit (story, feature, bug, optimisation, refactor, improvement). Branches are easy to make and easy to merge when you use git (and you apply some of tips further down in this post).

  2. Never Merge Your Own Branch
    This helps to ensures that code is in fact reviewed. If you are caught merging into master, you will order pizza for the whole team.

  3. Review Work in Progress First
    When you are finished with a task, you notify the other team members that your work is ready for final review. Then you review existing branches. Before picking up a new task, you look at all open pull requests (including unfinished ones) and review the changes since the last time you checked.

  4. Merge responsibly
    Merging a pull request is the responsibility of the whole team. A pull request can not be merged when someone in the team does not understand the code or the reasoning, or does not agree with the solution.

(Note that these rules are starting points. Figure out what works in your team, adapt continuously.)

The Benefits

Having multiple sets of eyes review a pull request before it gets merged to master or an integration branch, is a great way to catch defects early. At this time, they are usually still cheap to fix.

There are however much more important benefits. Instead of individual developers, the team is responsible for the internal and external quality of the code. This is a great remedy against the blame culture that is still present in many organisations. Managers or team members can no longer point fingers to an individual for not delivering a feature within the expected quality range. You tend to become happier and more productive, knowing that the team has your back. You can afford to make a mistake; someone will find it quickly.

Another effect is something called ‘swarming’ in Kanban. Because you are encouraged to help out on other branches before starting your own, you start to help others finishing work in progress. Stories are finished faster, and there’s a better flow throughout the system. Especially when stories are difficult, or when stories block other stories, it’s liberating to have people come and help you to get it done.

And of course, there’s all the benefits from the clear sense of code co-ownership. It’s invaluable to have a team where everybody knows what the code does, why it’s designed that way, how everything fits together. It also reduces the Bus Factor: no single team member is a bottleneck. Best practices are shared, and the code is more consistent. Opportunities for reuse are spotted before lots of duplication happens.

In short, pre-merge code reviews grow the team’s maturity.

Making It Easier

Reading code is hard, much harder than writing it. Here are some ideas that I have found to make things easier.

I’m assuming in this post that you use GitHub, but there are other solutions, such as ReviewBoard, BarKeep, Phabricator, and Gerrit. I did a little bit of research last year, and felt that GitHub was the best tool, but I have no hands-on experience with the others. YMMV. The important factor for me is that you can review stories or branches as a whole, and comment inline. (Update: @tvlooy suggested GitLab.)

Pitfalls and anti-patterns

As usual, I will keep adding to this post as I find more patterns. Got tips?

Update: Apparently Phil Haack blogged about code reviews almost at the exact same time. He has some great tips like keeping a checklist, focusing on the code and not the author, and stepping through the code. Worth reading!

Follow @mathiasverraes on Twitter.

Domain-Driven Design workshops
in London, Amsterdam, Copenhagen, Ghent...

Blog

Atom Blog feed

Creative Commons License This work by Mathias Verraes is licensed under a Creative Commons Attribution-NonCommercial-ShareAlike 4.0 License.