Eight Top Tips for a Tip-Top Code Review

Code reviews should be an essential part of a team’s development process. The immediate benefit is that they’re an opportunity to audit the code that has been written. The secondary benefits are just as important, though. A team that shares knowledge and supports each other is invaluable.

Reviews can be quite a daunting task, especially when you haven’t done many of them before. Preparation is vital — know what you’re looking for before you start looking for it.

“programming language codes” by Markus Spiske on Unsplash

Before You Get Started

Is This a Bad Time?

Before you start reviewing any code, ask yourself if this is an appropriate time to do so. There are two sides to this, and the answer depends on your team’s processes and your own schedule.

First of all, is the code ready to be reviewed? What needs to have been done before the code is reviewed, and have these all be done? For example, at FlexMR, we generally don’t review code until the task has passed acceptance testing and any dependent PRs have also been reviewed and merged. There are exceptions, of course. The code review process, even informally, can be a great tool for assisting a colleague that asks for help.

Secondly, do you have time to do a review right now? The time required can be difficult to estimate. While there’s a correlation between the size of a PR and the amount of time needed, there are too many other factors to consider use this as the only variable. You mustn’t rush a code review. It won’t do you, your colleague, or the code any favours. If you know you have a review to do, set a reasonable amount of time aside to do it. It’s often helpful to block off a part of each day (such as first thing in the morning, or immediately after your lunch break) and use that time to work through your reviews.

Use a Sensible Code Review Tool

Gone are the days where we’d have to do reviews via email. There are loads of tools available, so go find one that works for you and your team. Consider having your reviews be open so the process can be observed by other members of your team, in addition to just the author and reviewer. This gives the opportunity for everyone to see what changes are being made, and the thoughts that went in to writing and reviewing those changes. Thinking about how you would solve a problem and then comparing that to the solution offered is a great learning exercise.

At FlexMR, we do our reviews through GitHub using their PR review tool. It doesn’t have the most extensive set of features, but it works really well for us. It slots straight in to our development and CI processes, and we have alerts and reminders piped to us via Slack.

How to Request Changes Positively

Some teams have adversarial, sometimes even hostile, code review practices. Perhaps there’s an argument for doing so in projects where you don’t know the collaborators as you can’t be certain of the the author’s intentions. Think Open Source, or projects with outsourced development. For in-house teams, there’s no need for it. If you don’t trust your team to have the project’s best interests in mind, then you have problems that being grumpy in code reviews aren’t going to solve.

Don’t act like your colleague wrote a piece of code in a certain way just to upset you. Unless they were having a bad day they likely put in a decent amount of effort. If the end result isn’t up to scratch it’s probably due to inexperience, missing knowledge, or they just made a mistake.

So how do you point out code that needs improvement? Calmly explain what the problem is; why it’s a problem (if it isn’t obvious); then offer a suggestion on how to fix it. Depending on the issue, the suggestion could be as simple as linking them to relevant documentation. For non-trivial issues, a code example can be a big help. Perhaps even offer to talk them through it if it’s a particularly complicated issue.

Most importantly, keep your comments open for discussion!

It’s Review Time — What Should You Be Looking For?

1. What am I looking at?

Before you start looking at the code, ensure you have at least a basic understanding of what problem the code is trying to solve. As mentioned before, we use GitHub to review PRs. Each open PR has a quick summary of the problem and solution. It also has links to where a demo of the solution can be seen, along with a link back to the issue tracker with the original description of the work.

If it’s not immediately clear what the code should be doing you’ll need to ask the author for clarification.

2. Whitespace Changes and Other Superfluity

Ideally, the only changes in a PR should be those completely necessary for solving the problem at hand. Superfluous changes, of which whitespace tweaks are a perfect example, have a negative impact on the integrity of the commit history. Being able to associate a change to a time and reason for a change is critical for tracking down bugs and the general maintenance of large applications. GitHub’s Atom has a feature which strips all unnecessary whitespace from a file when it is saved. This feature defaults to ‘on’, so be careful if you use it. Please consider turning this feature off.

On the other hand, new whitespace issues should not be getting introduced. If you see any during a code review, please do point them out so the author can fix them.

Where do you stand in the Tabs vs. Spaces debate? It doesn’t really matter. As a team just pick one and stick to it.

3. Secure Coding Practice

Information Security is a hot topic in software development. As a developer and code reviewer, security should be in your mind at all times. When reviewing code, think about ways that it could be exploited by people with bad intentions.

As a minimum, ensure you’re up to speed with the OWASP Top Ten. Learn how these vulnerabilities look in the tech stacks that you work with, and learn how to protect against them. Be diligent about them during reviews.

Your language/framework of choice may have tools available that can help with ensuring your code is secure. For example, Ruby on Rails has Brakeman and Python has Bandit. Make use of them, but don’t rely on them to do everything for you.

4. Adequate Test Coverage

Are all changes to the code appropriately tested? New features means new tests. Bug fixes means new tests. New UIs or API endpoints mean new tests. In general, if code is being added or changed, there should be some tests to go alongside it. They shouldn’t be added willy-nilly, though. Tests should be useful — at the core of it, they need to prove that a change is working as expected and protect against future regressions.

Your testing strategy should be decided with your team. There are many different approaches so you need to have a discussion and decide what works best for you. Once your team has a plan, ensure it is being followed during your code reviews.

5. Missed Connections

There are some changes that should always happen in pairs. Some of these can be really easy to miss in review. For example, in the context of a Rails application:

• Changes to db/schema.rb should have corresponding migrations.
• Changes to Gemfile should have corresponding changes to Gemfile.lock

If you do code review before your acceptance testing, then you will also need to be on the look out for the more obvious ones. A new method on a model will need new unit tests. A new controller will likely need a new entry in config/routes.rb .

Don’t forget about documentation! If the behaviour of a documented piece of code changes, the documentation needs to change too.

6. Potential Impact on the App’s Performance

Having your application work as quickly as possible is generally regarded as being a good idea. It’s better for the users, and it’s better for your system infrastructure.

Be on the look out for ways the code can be optimised. Calls to the database are a good starting point. N+1 queries can significantly slow things down and are often easy to see and suggest fixes for. Learn about when your database of choice will lock tables to perform a read or write, and what you can do in your application to minimise any negative consequences of this.

If the code is handling a lot of data, is it being smart about how it retrieves and saves it? If it’s retrieving tens of thousands of records at once, would it be better served doing them in smaller batches? As well as the time required for retrieving the actual data, if you’re using an ORM you’ll also need to factor in the time and memory requirements of initialising those objects.

JOINs can be another killer if implemented badly. Check that the JOIN itself is beneficial. It can sometimes be easier and quicker to run separate queries. Naturally it depends on the complexity of the JOIN and what you’re doing with the data. Just keep an eye out, and when it doubt benchmark it.

Don’t neglect your non-DB related code, either. Get familiar with how to write fast code in your language of choice and look for how this can be applied during your reviews. Most tasks can be solved in a variety of ways, and some are faster than others. You won’t necessarily knock out the fastest way possible on your first attempt, but if you make the effort to learn some new techniques your code will improve, and you’ll have much more interesting things to say when reviewing your colleagues’ code.

See if your tech stack has any performance analysis tools you can integrate in to your work flow. The usual caveat applies: make use of them, but don’t rely on them to do it for you. For Ruby, have a look at Bullet and Fasterer.

One thing to keep in mind about optimisations — you might not need to squeeze every last little bit of performance out of your code. In general, you want to the code you commit to go fast, of course, but you must balance this against potential impact on maintainability.

7. Following Team and Language Coding Conventions

Every language has at least one style guide and every team has their own variation of it. Pick a style and stick to it. Consider adding some automated linting to enforce it before it reaches review stage, too. It’ll save your team loads of time.

Linting can only enforce the style guide on the code it can see. As a human code reviewer, it’s your job to look for the code that wasn’t written. Making appropriate use of Design Patterns is a prime example.

For example, if the PR adds lots of methods to a model, consider if they should be refactored in to a Service Object. If additional endpoints are being added to a controller, maybe it’s time for more controllers?

On the other hand, if the PR implements a radical design pattern that the team hasn’t used before, that should be brought up for discussion. Trying out new ideas is great and should be encouraged, but it should be done with the knowledge of the team since they’re going to need to fix bugs in it sooner or later.

Does your team have a process for naming things? Naming things is hard, but it’s good to be consistent. If several of your models can have a ‘name’, is the attribute called name on all of them? Or is it title on some of them? What about I18n keys; or cache keys?

Ideally you’d decide all of this in advance and document it. Then you can refer back to it, both as a reviewer and coder.

8. Opportunities to Share Knowledge

Code reviews are a fantastic opportunity for both sides to learn from each other. Forget the idea that learning is a one-way street of junior developers learning from senior developers. You can learn new things by paying attention and asking questions. Interesting people write interesting code, and interesting code leads to interesting conversations.

Once you’ve reviewed a lot of code, you’ll have no doubt experienced times where the author has solved a given problem in a way that you would not have considered doing. With programming, there isn’t just a right way and a wrong way. There’s loads of right ways, and there’s even more wrong ways.

If you do see something that looks wrong, don’t just say that it needs to be changed. Point out what is wrong, and offer advice on how to make it right. If you feel a piece of code is completely off the mark, offer your time to the author to help them get it to where it should be. Maybe ask them how they arrived at their solution. It might make a lot more sense to you. They were maybe making allowances for some unexpected edge case or dodgy data. Maybe they just got it wrong. Either way, there’s opportunity for you both to learn from this.

If the code looks fine, are there any ways it could be improved? If a junior developer has been cranking out some consistently good work, perhaps it’s time to give them more of a challenge. Could their algorithms be optimised? Are they making the most of their database queries? It could be that they’re ready to learn some additional skills, or to double down on what they already know.

If you have an open and friendly approach to code reviews, you’ll often find yourselves sharing interesting bits of code with the wider team. This can lead to some enjoyable conversations. At FlexMR, these conversations have often resulted in adopting new approaches to certain issues, like agreeing to the use of certain design patterns or trying out new tools. They’ve also lead to some fun conversations about how we’d solve hypothetical scenarios like how to validate that a password meets the security requirements without using regex; or how we could support Emoji unicode characters on a database system that would reject them.

TL;DR

• Don’t be a jerk — friendly code reviews are better for both the team and the project.
• Know in advance what you should be looking out for.
• Decide important things in advance. Your testing strategy and coding style should be priorities.
• Any comments you make should be open for discussion. Ask and answer questions as best as you can, everybody benefits.
• Learn from each other.