Code Without Review Is a Waste of Money

If you’re not already reviewing your team’s code before it’s merged, you should be! And they should be reviewing yours. There are many benefits and few drawbacks to regular code review. Here are some tips to help with the process.

Make it a part of your official workflow

I can’t stress it enough: No new code should go into your product without a developer review. As long as there is more than one person on your development team, you should require code reviews for every piece of code. Most of you probably already use a distributed version control system such as git, in addition to a repository host, such as Github or Bitbucket. These services provide easy ways to create “pull requests” (PRs), or requests to pull some new code into the primary branch of code, and advanced tools for reviewing these PRs, which makes it easy to integrate code review into your daily workflow.

Click here to read more about PR reviews in Bitbucket.

Click here to read more about PR reviews in Github.

Be Critical, with Tact

It’s crucial to not hold back during your code reviews. No one benefits from a meek code reviewer, and no one less so than the product users.

Go into every code review with a critical eye and a tactful choice of words. Above all else, though, be thorough. Code belongs to the entire team — and the entire team is responsible for both good code and bad code — so make sure you only approve PRs that you stand behind and will fight for alongside the original author.

A quick note about tact: Know your audience. On the one hand, careful, tactful comments may be necessary depending on the communication style of the author of the code. On the other, spending too much time crafting your feedback and explaining your reasoning to spare an author’s feelings can be a real waste of time. The most important thing is to make sure your tact doesn’t muddy your intentions. If something about code needs to change, make sure it’s made clear to the author. “Do we want to call this function here?” is tactful but might leave room for time-wasting back-and-forth in the PR comments. “This function call can be deleted” is to the point, but might leave the author wondering why. In the end, it depends on the developer and the culture you and your team have created. When in doubt, opt for a middle-of-the-road approach that is both to the point and briefly explanatory, at least until you better understand the author’s communication style: “This function call can be deleted because it will never be called.”

Don’t Just Get Used to It — Embrace It

If you’ve never experienced aggressive code review before, it can be jarring.

For those of you who have experience with code review, you know the story. You worked all sprint, putting in extra hours, thinking about the problem as you lie in bed at night. You coded until your fingers hurt, wrote tests, eliminated lint issues, and you’re feeling great because you managed to get it in before the sprint ended. Then, the code review occurs. Now you’re staring at a wall of comments from a variety of developers, each of whom is particularly concerned about their own pet peeves and areas of expertise. To make it worse, they’re all correct. You’re going to have to rewrite so much of this code, not to mention update the tests. And after that, you might still get another round of comments (or two or three). Sometimes it can last days. Days after you thought you were finished, you’re still working, and the sprint was over days ago. If only there hadn’t been so many different people reviewing — or better, no review at all — then the code would already have been merged and we would have made the sprint. And besides, the code worked just fine (for the most part).

If you recognize this story, you probably remember that early sense of dread of having to rewrite so much code. But a few things always happen after that first review.

First, and most importantly, you get better at writing code! Quickly. One surefire way to lessen the fear of code review is to not make the same mistakes again. Sure, some lessons are harder to internalize and can be more difficult to recognize than others, but many are easy to spot and even easier to remedy. In addition, a lot of the early “mistakes” are simply company preferences. Hopefully most of these preferences are caught by a linter, but inevitably some won’t be, and you just have to memorize them. For instance, at BetterCloud, a rule of thumb has emerged regarding how we group dependency imports at the tops of our Javascript files. This type of un-enforced lint rule can make up a large amount of PR comments for newer developers. Part of being a professional developer, like being a writer for a TV show, is the ability to adapt your “voice” to that of the company’s.

Second, you realize that with coding, many heads are always better than one. One of the most rewarding aspects of writing code for a living is that there is never a shortage of new things to learn. I love spending time writing what I think is a clever algorithm, only to set it loose on my colleagues and have them pick it apart. The final product is often simpler, more succinct, more efficient, and easier to read than what I was able to come up with on my own.

Third, it becomes clear that reviewers are learning as much from your code as you learn from their comments. Reviewing code is as helpful — if not more — for those reviewing it as it is for those writing it. The reviewers are forced to read and parse code written by someone else, think critically about the patterns and style of that code, and formulate (tactful but succinct!) comments and change requests. Reviewing someone else’s code, especially if the person has more experience, is one of the single best ways to learn on the job.

Don’t Allow PRs to Grow Stale

Letting a PR languish without review is a mistake. If a PR goes too long without a review and a subsequent merge, the potential problems with the code and the resulting wasted time getting the PR in good shape grow exponentially.

Merge conflicts are more likely to crop up, newer code that depends on the PR is held up, and fixing the code takes longer because the extra context-switching means the author has to re-remember the original intent of the code and must ramp up on the ins and outs of the logic that is now perhaps multiple days old.

All of this adds up to wasted time and money. Make sure you code review early and often. Work it into your daily routine and don’t put off any PR for longer than a few hours. This constant review will hone your skills and the skills of the author, and ultimately will help to avoid time-wasting mistakes making it into your production environment.

Lose the Ego

It is of supreme importance that developers leave their egos behind when facing a code review from someone else. It’s particularly important for tech leaders to model the art of gracefully accepting a code review. The team will follow the leader’s example, so if you’re in a position of power, make sure you happily embrace your own code being reviewed as much as you embrace reviewing others’.

Expect to have issues with your PR. Be ready for it. In fact, if you’ve created a PR of any respectable size and don’t receive any comments in a PR review, something is probably wrong with your code review culture. Make a habit of thanking the reviewers for their insights and quickly and eagerly adopting their suggestions and updating the PR with requested changes.

Integrate, Integrate, Integrate

If you’re using one of the more popular code repository and PR review solutions such as Bitbucket or Github, there are a wealth of options for integrating your version control management with other parts of your workflow.

Perhaps the two most common and important integrations are with your issue and feature management tool and your build and deployment tool.

At BetterCloud, we use Bitbucket for version control and Jira for managing issues and features. We track Issues and features with Jira “tickets,” and it’s helpful to associate these tickets with Bitbucket PRs. For instance, once a PR is created, the associated ticket is flagged as “ready for review,” which notifies our functional analyst that a PR is ready for testing. These types of links between tickets and PRs and the mirrored state changes among the two systems can and should be automated.

Click here for more information about integrating Jira with Bitbucket.

If you’re using some other version control system such as Github, or some other issue management tool like Trello or Basecamp, there are plenty of options for integration.

Use Commits to Speed Up Subsequent Reviews

After a PR has gone through a round of reviews, and the author has made the requested changes, the PR is updated. This update means the PR needs to be reviewed again. This can be annoying, particularly if the PR was very large. It may have taken 10 or 20 minutes to adequately review the PR initially, and now it needs a second review?

This is where a well-groomed list of commits comes in handy. Rather than reading through the entire PR again, view the list of commits and only review the commits that have been added since you first reviewed the PR. This not only speeds up the review, but it also helps the reviewer to see how the author thinks about code, giving useful insight into that person’s coding and problem-solving style.

Develop Good Commit Habits

To maximize the usefulness of your commit history, it is important to remember that once a commit is merged into your master branch, the only remaining use for that commit is as a historical record. For this reason, commits should be optimized for readability. In practice, this amounts to little more than making commits frequently and following good etiquette for writing your commit messages. It is typical to see junior coders commit in fits-and-starts, and mature coders commit usefully and holistically. Making these adjustments can dramatically improve both the usefulness of your commit history and the code itself. By thinking towards a readable commit history, you think about the code more holistically. This vantage makes you a better coder!

It Provides a Record

At BetterCloud, our code lives and dies by reviews. No matter the looming deadlines, code doesn’t make it into production without multiple approvals. We are dedicated to the practice, and as a result, we do a pretty good job of keeping a record of why we make the coding decisions we do. From time to time I find myself running into a piece of code that is perhaps under-commented or not as simple as it could be. In these cases, I can almost always find a PR where the code was introduced and, just as often, I will find comments about the reasoning behind the code. This happens often enough that we have more frequently been reviewing PRs with the comment “Let’s move this explanation into a comment in the code.”

Not Doing It Wastes Money

In the end, we code review not just to get better, but to make the product better. And more importantly, to make the product better sooner. By avoiding the creation of avoidable tech debt, we save time and money, allowing us to spend that time and money on creating more value for the customer, rather than fixing (or worse, ignoring!) tech debt that could have been avoided with a healthy code review.

If it feels like a PR is taking longer to finish because of review, good! That time is nothing compared to the time it will take later to re-acquaint yourself with the logic, create a new branch, a new PR, and a new ticket, and to merge and test this code. It’s also possible that the company will fail to see the importance of paying down the tech debt, and you’ll never get around to fixing it, forever feeling the pain of it and working around it, compounding the problem. It is much much easier to fix the problems during the initial development than it is to return to the problem later.