Writing for Code Review

Craig Davis
Apr 1 · 6 min read

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

Image for post
Image for post
Figure 1. Any idea what behavior this changes?

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 . 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 . 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 we don’t even have to change the any existing code. Here’s the diff for adding the new feature.

Image for post
Image for post
Figure 2. Adding sales tax with a boolean flag

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.

Image for post
Image for post
Figure 3. Adding a methods instead of a boolean flag

Imagine instead that we added a new method for calculating a total with tax. As an alternative, we could have changed the to and then added a new . 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.

Image for post
Image for post
Figure 4. This is the alternative to Figure 1, and the diff has no ambiguity

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 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.

Image for post
Image for post
Figure 5. To fully understand this change, we have to know the defaults of .

This could give us a diff that is also very clear in intent!

Image for post
Image for post
Figure 6. There is no confusion as to what is being changed.

Lint Everything

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.

Image for post
Image for post
Figure 7. The actual change is from `getExpressionLanguage` to `getCurrentLanguage`.

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 .

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.

Image for post
Image for post
Figure 8. Why are are adding this?
Image for post
Image for post
Figure 9. We’re adding it to the list of characters that are incompatible with the editor.

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:

Image for post
Image for post
Figure 10. Was this a change of behavior, or just an optimization?

Any maintainer to come across this code later will be forever puzzled about it. Should it be deleted? Perhaps uncommented? Mysteries waste time.

In Closing

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.

Diff Screenshots

All of the screenshots in this article were done in iTerm2 with diff-so-fancy. The tool is fantastic for creating human readable diffs on the command line. Don’t miss the pro-tips for setting it up with using pre-configured search patterns to make skipping through a diff very quick.

Welcome to a place where words matter. On Medium, smart voices and original ideas take center stage - with no ads in sight. Watch
Follow all the topics you care about, and we’ll deliver the best stories for you to your homepage and inbox. Explore
Get unlimited access to the best stories on Medium — and support writers while you’re at it. Just $5/month. Upgrade

Get the Medium app

A button that says 'Download on the App Store', and if clicked it will lead you to the iOS App store
A button that says 'Get it on, Google Play', and if clicked it will lead you to the Google Play store