As developers we all know that reviewing code is a beneficial and necessary process. Remember: Code review can help make your entire team more efficient, your software more maintainable (and maintenance is 70% of the cost of software), and your job more rewarding! At least in theory! In practice, nobody really wants to review code—a task perceived as tedious and time consuming.
Everyone here on the Tuleap team shares a strong belief that code review is an investment that pays off in more than one way. But we didn’t just wake up one morning believers! We have been reviewing code for more than 8 years. Our code review process has gone through multiple iterations, and with each iteration we have made improvements.
To give you an idea of what code review looks like for Tuleap software, in 2018, our 20 developers and reviewers submitted 38 commits per work day (for a total of 8,581 for the year). The median review time between a push and a review is 1 hour, the average time, 16 hours.
This article shows you some of the things we’ve learned in eight years reviewing code.
As Enalean’s CTO and Tuleap product owner, I have seen the process from all angles. Don’t miss this chance to benefit from eight years of experience in the time it takes you to read two short blog posts. You’ll learn why everyone has to get involved, why talking to each other matters, and why there’s no worse time to skip code review than in an emergency!
There’s no such thing as too many cooks
In other words, everyone on the team, regardless of where they are in the org chart, has to be capable of reviewing. And there’s no better way to learn to review code than to…review code!
The first person to review any code should be the developer that pushed the code. Step back and take a critical look at your own code. A pre-commit code review is essential to catching those inevitable mistakes and is the precursor to empathy and open communication between peer reviewers.
Experienced developers can benefit from reviews by junior developers—and even interns. This is the magic of the beginner’s mind…take advantage of it! Junior developers, on the other hand, can learn a lot from reviewing more senior peers’ code. Yes, an intern refusing a senior developer’s commit can be a traumatic event. But sometimes you have to take one for the team. Ego has no place in the code review process.
It takes time to build up your code-reading muscles. Think of code review as a vital part of your training routine. With each workout your team will become more efficient…and your releases will be more robust and maintainable.
This is what a developer told me about code reviewing:
I spent an entire month doing nothing but reading code—something I never imagined doing. It was tough going, but I really felt that I had improved as a developer and that I had made a concrete contribution to getting the release over the finish line.
The integrator gets the final word and is authorized to integrate the reviewed code.
Sometimes you need to turn off the tools and talk
Code is more art than science. Some parts of the code review process can be automated (Git Pull Request and Gerrit are two of the tools you can use), but you do get to a point where human intervention is a must. In fact, code review is a great way for development teams to learn (or re-learn) to talk to each other.
A good code review process will facilitate communication and leave detailed records. And the benefits are double: the more specific and detailed your code annotations and commit messages are, the easier it is for your reviewers to understand your rationale and for the team to retrace the breadcrumb trail to a bug or regression in the event of an issue later on.
Effective communication means giving each other the benefit of the doubt (the developer who pushed the code did his or her best) and providing specific and constructive feedback.
But there’s one caveat: If your team is already dysfunctional, the code review process will only amplify what is broken. Fix your team dynamics first, and then bring code review into the mix!
Emergency? Don’t skip the review!
Resist the temptation not to review code in the event of a “code emergency”! Emergencies require particular attention—and only a thorough and thoughtful human code review will do. In fact, you should have a specific code review process in place for urgent and/or critical issues. Plus, as your team gets better at reading code, reviewing code, and communicating effectively about code, “emergencies” will occur less frequently and, when they do, you will be able to resolve them calmly and efficiently, retracing your commit annotations and messages until you find that bug or regression!
Start fast (like now!) and make adjustments often
If you are ready to introduce your team to code review, the first step is to get started! Seriously, choose your tools (Slack, email, Gerrit, etc.), set up a simple process, and go! Then make adjustments often.
- Get everyone involved
- Talk to each other
- Resist the temptation to skip CR in an emergency
- Start fast and adjust often
We looked at some of the conventional wisdom about code review (it is time consuming and tedious) and shared our insights gained over eight years in the code review trenches.
When it comes to reviewing code, there is a lot of talk about tools. And having great tools is important. The right tools used in the right way will help you create, maintain, and improve efficient code review processes.
In this part of the article, we will go beyond tools and delve into some best practices that will help make your commits as effective as possible. Because (yes!) the commit is the key to great code review.
As Enalean CTO and Tuleap product owner, I have been in the code-review trenches for more than eight years. Read on to learn how and why you should make your commits great.
The commit: Size is not the only thing that matters!
As we discussed, the developer who pushed the code should always do his or her own pre-commit review. Once that is done, it is time to think about the commit itself.
And, yes, size does matter! There are a few ways you can right-size your commits. First, always remember the Single Responsibility Principle. In other words, by making sure each commit only changes ONE thing, by not mixing formatting and functional changes in the same commit, and by using a dedicated commit for refactoring, you will have gone a long way to right-sizing your commits.
Size does determine in part what goes into each commit. However, how you build your commits is also crucial!
There’s no time to be brie
Interestingly, the commit message is not the place to economize on words. Why? Because the commit message performs two important functions. First, it tells your reviewer why you did what you did. Second, in the event of a bug or regression down the road, commit messages are like a map that will lead you to the problem quickly and efficiently.
So, what makes a good commit message? Well, the title should be short yet descriptive. Compare this to your email inbox. How annoying is it when you are looking for a specific message and you have to scroll through dozens of vague email subject lines (like “meeting” or “deadline”)? Commit message titles follow the same principle. Think of how the messages will be used later and how you can make the users’ lives easier!
The body of the commit message is where you can really express yourself. Don’t just say what the code does…let the code speak for itself! What you do need to articulate is the rationale behind your commit.
Refusing a commit doesn’t have to hurt!
The same communication practices also apply to the reviewer. Remember, as we discussed in the last post, assume that the developer did his or her best and review with kindness! Be specific and meaningful in your comments. Referencing developer documentation or internal or external sources will help you be as effective as possible. Plus, providing objective justification for your review process has much less of an “ouch” factor than telling the developer “This code stinks!”
Cruising speed: continuous improvement
Once you are up and running, you manage code review just like you would any other workflow. You will need to constantly make adjustments to reach the ideal balance between throughput and latency to ensure that new features are delivered within an acceptable timeframe and that build costs stay within reasonable limits.
- One commit = one change
- Write descriptive titles and messages
- Refuse commits with kindness
- Aim for continuous improvement
Get in touch with me
As CTO of Enalean and Tuleap product owner within Enalean, Manuel’s mission is to keep Enalean’s tech team happy. This means providing our developers and other tech staffers with a challenging and rewarding work environment guided by a single philosophy: to be accountable to our customers in everything we do. Manuel spends much of his workday…you guessed it…reviewing code! He also dabbles in systems administration, continuous integration, and other software-engineering tasks.