I believe that we can learn to write code that naturally creates easy to read pull requests with small and concise differentials. Pull Requests are an important step in development and act as a final review of changes and give developers one last chance to collaborate, look for bugs, and consider improvements before a project is shipped. With some small changes in how we write code, we can make our diffs easier to read and understand. In this article, we’ll look at a few approaches to create easy to understand Pull Requests.
Don’t use Boolean flags
In the example above, it’s not clear from the diff what the PR is going to do. Let’s take a moment to see how we might have gotten to this point. In this first example, we are imagining a billing system that was written without consideration for sales tax. In this system we might have
SalesCalculator::calculateTotal(Cart $cart). At a later date we need to add a way to apply sales tax to the cart total. It would be easy to simply change this method signature to
SalesCalculator::calculateTotal(Cart $cart, bool $addSalesTax = false). This fix is expedient and in some ways seems like an ideal solution. We’ve made it simple and easy to indicate if we need to add sales tax to an order, and thanks to the default value of
$addSalesTax we don’t even have to change the any existing code. Here’s the diff for adding the new
Let’s assume that a month later we need to implement sales tax in another location. The diff would look like our first figure 1 above.
Anyone reading the diff has no context for the change — the diff in no way indicates what is changing. You simply can’t judge the change without going to the method definition and finding out what the boolean flag parameter is for. We would have to explain the change via comments in the PR, discussion, or by having the reviewer refer to documentation. We’ve wasted some time.
Imagine instead that we added a new method for calculating a total with tax. As an alternative, we could have changed the
SalesCalculator::calculateTotal(Cart $cart) to
SalesCalculator::calculateTotalWithoutTax(Cart $cart) and then added a new
SalesCalculator::calculateTotalWithTax(Cart $cart). With this approach, our PR review would have looked like figure 3.
The intent of the change in the PR is completely clear, and there is absolutely no confusion what this change set will do when it’s applied to your production branch.
We can get a similar benefit with another approach, and it’s one that is especially useful if you don’t have the ability to change the interface of a class. Let’s assume that we’ve got a third party class that has a
SalesCalculator::calculateTotal(Cart $cart, bool $addSalesTax) method that we must use. We’ll also assume that we are not using an adapter class to solve this problem. We could add new constants to specify the intent of the boolean.
This could give us a diff that is also very clear in intent!
Whitespaces and formatting changes make pull request diffs noisy and error prone. In the following screenshot, the PR would introduce a syntax error. You’ll notice that it’s quite hard to find.
If you lint all of your files with an established code style guideline you’ll be able make sure that all of your files meet a standard. In doing so, you will no longer suffer through individual developers enforcing their own preferred style onto a file as part of a diff. Instead, you’ll get uniform and easier to read code that has smaller tidier diffs. PHP Code Sniffer has scripts for both checking for violations and fixing them automatically.
If you must do some reformatting during a PR, there are a couple of useful tools that can make reviewing the code less error prone. Github offers a great shortcut to view a PR without whitespace changes highlighted. You can do the same via the command line with
git diff -w (--ignore-all-space).
Consider naming instead of commenting
Comments should document any unclear code. If the why of code is not clear, a comment should be added to clarify the intent of the code. However, this introduces two sources of truth. During code review, developers will have to read both comment and code and then ensure that they both match in intent. This confirmation step will slow down code review.
We can instead consider rewriting the code so that the why is more self-evident. For instance, you may be able to replace a comment with better naming or other changes and remove the need for the explanation via a comment. Doing so can allow the comment to be removed and prevent the comment and the code from potentially drifting apart over time. We should remember that comments have to be maintained too!
Let’s look at two diffs and consider which we’d rather review.
If we refactor the code so that the intent is clear without the comment, we can completely omit the comment and still have clarity. Any time that you prepare to write a comment, take a moment to consider if you could express the same idea by changing code instead.
Delete commented out code
Your source control system is your historical record of the code. You should ruthlessly delete unneeded code instead of commenting it out. I urge you to consider why you might be choosing to leave commented out code in your project. Imagine seeing the following diff and trying to decide why this change was made:
Any maintainer to come across this code later will be forever puzzled about it. Should it be deleted? Perhaps uncommented? Mysteries waste time.
Code is read more than it is edited and it should be written in a manner that makes it quick to read and understand. We can improve code even more by writing with future maintenance in mind. By considering what a diff will look like as we write code we can keep our diffs small and easy to understand. Small and easy to understand diffs will lead to your team shipping faster and with fewer bugs.
All of the screenshots in this article were done in iTerm2 with diff-so-fancy. The
diff-so-fancy tool is fantastic for creating human readable diffs on the command line. Don’t miss the pro-tips for setting it up with
less using pre-configured search patterns to make skipping through a diff very quick.