December 2017
100 Duck-Sized Pull Requests
This post aims to answer the age old question: “Would you rather fight 1 horse-sized pull-request, or 100 duck-sized pull requests?”. It explains some of the guidelines we follow to ensure we’re writing better code as a team. It was all inspired by a simple tweet.
10 lines of code = 10 issues.
— I Am Devloper (@iamdevloper) November 5, 2013
500 lines of code = "looks fine."
Code reviews.
The Problem
I realized we were not getting the most out of our code reviews at Fullscript. Frequently we would have long standing feature branches. They would grow to thousands of lines of code. Not only was providing meaningful feedback extremely time consuming - it was daunting.
Developers would put off reviews until they had time/energy and the development process would come to a halt. Often times reviewers would feel pressure to rush a review, glance through the code at a surface level, and write something like: “Looks good to me! 👍”
Even when reviews were done well, it was often too late in the process to allow for major refactors. Early architectural mistakes had already taken root, and the cost of correcting them were too high for a startup to incur. We’d layered weeks of work on-top of a shaky foundation.
Unfortunately, code wasn’t getting the level of scrutiny required. People weren’t learning. The review process was failing.
Great, so how can we do better?
When you ask for a review, you are asking someone else to share responsibility. They have to understand the problem, grok your code, and if all is well - hopefully give their stamp of approval. As developers we should be making that job as easy as possible.
Early & Often
Ask for feedback when it is most critical: early in the development process. This gives reviewers the chance to detect early design problems, and ensure that you’re building on a solid foundation. Your team can avoid costly rewrites later in the development process, or merging code with known flaws due to time or budget constraints.
Smaller
What do I mean by small? As close to zero lines as possible. Who doesn’t love to review a one line change? Limiting the size of the pull-request (read: lines of code) will have a significant effect on its’ approachability. Not only is it easier to thoroughly inspect line-by-line, but it’s significantly less time consuming. You’ll be able to merge code more regularly and feel confident that it’s quality.
Limited Scope
A big factor in reducing lines of code comes down to limiting the scope (read: feature set) of the pull-request to solve a single, or a few closely related tasks. Reducing the scope will significantly reduce the cognitive load on the reviewer. If you’re solving multiple problems within a single pull request - sorting out which bits of code relate to specific problems can become difficult.
Release Incomplete Features (but keep ‘em hidden)
Waiting until a feature is complete before releasing is a pretty common practice. Unfortunately this is a major cause of gigantic long-standing feature branches. As development continues on the master branch, it often results in merge conflicts, rebases, and other fun. Even when they are merged, deploying a large change-set can be nerve wracking for the team. This is especially true when it’s on a flagship feature or an upgrade to a major dependency.
Using a feature flipping tool, you can hide away incomplete features from your users. This allows you to continually merge code and release often and without fear. As development nears completion, you can begin granting access to specific accounts and early adopters. Deployments become less scary, and you have control over granting access to features as you see fit.
PS I highly recommend the gem flipper if you’re using Ruby on Rails.
Plan Ahead
Working under these constraints causes developers to break problems down into incremental deliverables. It helps avoid the temptation of jumping into development without a clear plan. This can take some practice, but gets easier and becomes a natural part of development.
Don’t Mix in Refactors
Commonly developers will notice problems and refactor as they go - and that’s awesome! But those fixes should be done in a separate pull-request. This ensures they are merged sooner and not tied to an unrelated feature release. If the original pull-request is delayed (or worse never released), all of those improvements would be held back or lost.
Try it
Code reviews are an integral part of building software within a team. They are a place for sharing knowledge, and act as a gatekeeper for code quality. Working with these contraints has helped ensure that we get more out of code reviews at Fullscript. It has resulted in:
- Quicker & higher quality code reviews
- Releases that are easier to test and much less terrifying to deploy
- Ability to easily revert & rollback changes when problems do arise
It sounds simple (and it can be), but it requires team buy-in and will take getting used to. If this message resonates with you, then I suggest having a conversation with your teammates about how to get started.
Please add any comments or questions below, or feel free to reach out on Twitter.
Also, this post is based on a talk I gave at a local meetup. It was originally inspired by a chat with Kevin McPhillips & Willem van Bergen on their approach to building software within a large team. Thank you guys!