One of the things we love at Media Suite is code review. After a feature is completed but before it is shipped to the user, at least one other person will review the code changes. We have found this ritual is an effective way to share knowledge and avoid bugs.
The practice of code review is ubiquitous across software projects in 2017, but the recorded history can be traced back to the linux project 1991. It’s a great demonstration of how it enhances quality and massively scales.
Back then, an email chain was the primary means of code review. These days we might use pair programming, Slack channels or GitHub. How we share it changes, but the benefits stay the same.
When we review code, we are checking for software defects. If we check it before we send it out into the world, bugs and faults are easier to fix, and it saves time wading through multiple bug reports from users.
Instead of “Button X doesn’t do Y” you get “function X is mutating global state, fix it scrub!”.
We also share our knowledge, allowing us to annotate code with context that will be handy down the track, and breaks down barriers to understanding code written by others.
It can also be done by one person at any time – no time-consuming meetings and paper trails required.
The most important piece of knowledge sharing however, is between your most experienced and least experienced developer. It is the fastest way to learn as a junior developer, and opens up so many opportunities for learning.
Brooks’ Law says that “intercommunication in a project follows n(n − 1) / 2 where n is the number of programmers on a team”. This has been common knowledge around software engineering since the 70’s. Code review breaks the rule. Having more individuals involved will increase the speed of delivery.
Github is a great way to do code review, but isn’t necessary. Email chains work just fine too.
When Should I Review?
When should you stop writing code and review? There are two guidelines we use. First, we always try and focus review on the piece of work that is most important to share knowledge on, this is done by keeping the amount of changed code small. The other is when you finish a block of work that makes sense.
Keeping the code change small is a way of tuning the amount of attention a change gets, an interesting phenomenon is that small code changes tend to get the most attention in review. If you hit someone with 800 lines of code, you’re significantly more likely to get superficial feedback. The sweet spot that works for us is about 200 lines of code change.
What If It’s a Big One?
If it’s major changes across many files, split up the review in to logical chunks. Separate refactoring changes from functionality changes. Annotate the code with notes to draw attention to the areas where there is a high risk of introducing bugs.
We use a one to ten rating system when commenting on a change, let’s take a look at how this is used in a real example…
Mark has annotated his comment with a 1/10 rating, meaning the comment is low priority. You can safely ignore this comment if it is not relevant. A 10/10 rating on the other hand is something so serious that it could cause critical defects in the software. It should be addressed and it usually leads to an offline conversation between the reviewer and reviewee.
In our process for code review, the creator of the change gets final say in ignoring or acknowledging comments. If they choose to, they can pull finger, and just merge the changes. In practice, this happens only in exceptional situations.
Who’s Involved?
As the author of a change, it is your decision about who to pull in. Sometimes it makes sense to tap your business analyst on the shoulder and ask a choice question about requirements. In other cases where the change is ops related, you may bring in the infrastructure guy for review. Again, the rule is a guideline, and the guideline is supposed to be broken. In different situations I have called on people specialising in operations, user experience or testing for reviewer duties.
Another role that we mix into the process is a “merge champion”. This person will operate as a kind of arse kicker, making sure changes get across the line before demo. We love this person, because a code change that sits in review for too long is likely going to be painful when merging.
Summing It Up
So we have a bunch of behaviours we encourage during code review, but there is one theme that cuts across every one of them.
The responsibility for merging code belongs with the creator of the review.
In other environments I’ve seen the process go wrong and turn into a formality where you’re just blindly trying to tick boxes to cover your butt. It can also turn into a slog of commenting, debating and fighting with your colleagues. In my opinion, the secret sauce is keeping ultimate merge rights with the creator of the review. It only takes one stupid typo getting into your code review to illustrate the advantages of the process. On the other side, your first one-liner review will teach you how much developers love the sound of their own voice.
Get someone outside the project to review your code. This works for us because we mostly use a common stack of technologies. It helps us reduce code ownership, and makes the productivity loss from switching projects minimal. Surprisingly, the “stupid questions” a cross-project reviewer asks, are usually an indication of a complex business rule or concept that needs to be well-communicated in the code.
Don’t be afraid to be a bit silly. As a mostly remote team, we don’t get a chance to fool around in person. Use your review to hone your banter. We’re all mature enough not to get unnecessarily distracted by it.
Use the review as an opportunity to learn, if you see something that is unfamiliar, say something! If the reviewee is too busy to answer, someone else probably will. The process is a conversation between everyone involved, not just 1-1 between you and the author.
The rules are really guidelines and the guidelines are made to be broken. The fastest way to change behaviour is by example. Be the change you want to see in the code review process! In my opinion the way code review works at Media Suite is a happy balance between rigour and flexibility.
Banner image: Photo by Una Laurencic
