What makes a good code review?

Start With The Right Attitude

Code reviews are a collaboration between two or more people. It's not an opportunity for you to show how superior you are, or how stupid the author is. Code reviews are not there to judge or berate people. The author should not take criticism personally, and likewise, the reviewer should be as constructive as possible. Code reviews should not be feared, they're opportunities to learn from each other, and improve quality. Treat each other with respect.

Code Reviews Begin With The Author

Like any relationship in life, there's always more than one person involved, and both the reviewer and the author are responsible for the outcome of a code review.

The reviewer is not there to fix your code for you. Treat reviewers with respect – they're giving up their time to help ensure the quality of what you've done, after all – so make sure you do all you can to ensure a code review is truly ready to be reviewed by doing a few simple things.

Plan And Prepare

A good code review actually begins before a line of code is even written.

The developer should make sure that they know what they're about to develop, and think carefully about how they plan on approaching it. You must understand the requirements of what you need to do, first and foremost, and clarify anything you do not understand before you even think about a potential solution.

Once you've understood your requirements, and thought of a solution, it's always good to get a couple of other developers together to go over it beforehand, preferably also including the one that is likely to review your work. Not all solutions are complex enough or impactful enough to warrant a full blown discussion, but getting someone to have a look at it before you code it can help pinpoint possible weaknesses.

You'll be amazed at what a difference this can make in reducing the amount of time it takes to get something through code review. Identifying the code reviewer early is important: the reviewer knows what's coming, and can spend some time understanding the work they will be looking at.

Also, remember to allocate time for code reviews when estimating how long your work will take. One of the major factors in poor code reviews is time pressure. Accounting for it early will help.

Good planning and preparation before a journey is paramount in helping you reach your destination.

Give A Good Subject And Description

So you've written your code and your tests (I'll assume you have, anyway). Your tests pass, and you're ready to open that pull request.

First, give your code review a good title that describes succinctly what you've done. Typically, I give my code reviews a title in the format of `<branch name> – Subject`. We name our branches using a fairly standard git flow naming convention using feature, hotfix, release etc., so an example subject would be `feature/91784 – Allow user to reset password`. In terms of style, stick to the same style as a git commit message: 50 characters in length if possible, use the imperative mood, and have no punctuation at the end of the sentence. Once it's merged, your git log will thank you.

For your description, outline what you've done, and why, if necessary. Use bullet points to make things easy to read and understand. Link to any related pull requests as a signpost to any dependencies. Be as simple and concise as possible.

Giving a good subject line and description will help highlight potentially important areas that a code reviewer needs to focus on. More importantly, it demonstrates your own understanding of what you've done: if you can't describe what you've done clearly and simply, this can be a warning sign to yourself that maybe you've not fully understood the ticket, or the code itself.

Be Your Own Code Reviewer

Before you even assign your code review to anyone, you should review it yourself. You can also use `git diff` on the command line, or use your IDE to view changes on each file you changed if you wish to check changes before even opening your pull request.

It becomes quickly apparent during a code review whether or not someone has actually reviewed their own work, and this can be exceptionally frustrating.

As an analogy, imagine you're a publisher, and an author has sent you a manuscript to read. You begin reading, and it's obvious that the author has not edited their own work: every line is riddled with spelling and grammar mistakes. The story is lost in a confusing, jumbled mess of repetition and plot holes. That publisher is unlikely to want to waste their time.

Likewise, a code reviewer doesn't want to waste their time on things so obvious that the author should've picked them up at first glance. It shows a lack of respect, and is a sure fire way of annoying your colleagues.

Code Reviews Are Not Just About The Pull Request

Once the author has handed over the pull request for you to review, it's time to begin. Except, don't start with the pull request. The pull request is only one part of a code review.

As the reviewer, you first need to know what you're actually reviewing. You need to read and understand the requirements for the work that was done: related tickets, user stories, business requirements, specifications. How can you say something works, if you don't know what needed to be built? It would be like reading someone's CV and approving them for employment without even knowing what position they were applying for.

This may seem tedious, but it's important to remember that the vast majority of software fails because of the requirements, not necessarily because of the software itself. Understanding what needs to be built acts as an extra check that the requirements are sound. If you can't understand the requirements, ask for clarity, and if you still can't understand, then how did the developer manage to code something? Was something discussed, and not documented? Ask questions of the developer, and others, if necessary, and get them to update and clarify any documents, as appropriate.

Reject Early If The Basics Are Not Done

Once you're satisfied you understand what needs to be built, and you begin the code review, it can be plainly obvious that the basics have not been done. If this is the case, reject the pull request, and send it back to the author with a brief note explaining that they need to ensure it's ready.

The fact is, time is precious, and it's not up to a reviewer to point out why CI Tools have failed, comment on obvious formatting errors, or identify code that an IDE is highlighting. These sorts of things should not require a reviewer to point out, and they add a lot of unnecessary noise, preventing the reviewer from focusing on the code itself.

Code Review What Is Not There

It's easy to only think about what is in front of you, but a reviewer needs to pay close attention to what you don't see.

• Make sure the code actually fulfils all requirements.
• Ask yourself whether or not they've solved the wrong problem. A simple misreading of the requirements can lead someone down the wrong path.
• Are there any negative scenarios that have not been accounted for?
• Are they catching and dealing with exceptions?
• Are there any tests? If not, why? And if there are, do they have good coverage?
• Have they forgotten to add logging somewhere that could be useful?

Working Code Doesn't Mean Good Code

So, they've solved the problem, and the code works. However, it's quite possible that they've gone about it completely the wrong way. This can be tricky to deal with, because there are many different solutions to a problem, but sometimes it can be glaringly obvious that the code can be refactored in a far better way. Catching these things early can save a lot of pain later.

Is It Simple?

Simple things should be simple, complex things should be possible. Approach the code with the point of view that perfection can be achieved because there is nothing left to take away. The simplest and best code is the code that isn't written. Can code be refactored into simpler, reusable methods? Perhaps you're aware that similar code exists somewhere else in the codebase, and it's the right time to consolidate into a single source of truth. Perhaps new classes can be abstracted out into something far more useful and reusable, and share some commonality?

Is It Optimal?

I'm a big believer that premature optimization is the root of all evil, but that doesn't mean you shouldn't pay attention to the obvious. Developers are often getting very accustomed to having large amounts of memory and powerful multi-core CPUs, but these things should still matter. Are there potential memory issues, or excessive CPU usage? In addition, are database queries optimal, and using good indexes? Are there queries within loops?

Is It Safe?

"First, do no harm." All code should strive to make systems safer and better: robust validation where necessary, handling errors and exceptions gracefully, making sure data is sanitized before being stored into the database, preventing sensitive data being dumped to the screen. Think about whether the code introduces extra technical debt.

Are They Using The Right Tools?

To a man with a hammer, everything looks like a nail. What technology is being used? Are they being used correctly? Has the code used any design patterns, and if so, are they the right tool for the job? If one wasn't used, could they have used one? Consider whether there are any anti-patterns within the code as well.

Do Names Make Sense?

This is far more important than people realise, and it's hard. Names convey meaning and understanding to other developers. If names of variables, classes, and methods are confusing or misleading, it will hinder future development and debugging.

Are Messages Meaningful?

Make sure messages are clear, and simple. This includes messages from exceptions, logging, and API responses etc. Pay attention to comments as well. Are they accurate, useful, and meaningful? Do they just repeat what the code already says? And if there are any todo items, ask whether or not they've been raised as tickets on your backlog, or brought to the attention of others.

Also, one thing to be aware of: in some systems, storage of log messages costs money. There's no point in storing thousands of lines of messages if they are not actually helpful.

Does It Fit Within The Wider Ecosystem?

Code does not exist in isolation. It forms part of a much bigger picture. As such, you should think about how this code fits within the rest of the codebase. Every codebase has its own idiosyncrasies and standards. Have they been consistent in their approach compared with the rest of the codebase? If not, find out why. Should parts of the code exist in a different package? Does it make use of existing code?

Have They Been A Good Scout?

We often work on old, sprawling legacy systems that existed long before our time. There is always a temptation for someone to go in and do the bare minimum of changes to some code in the fear of touching and breaking something.

However, fear makes good armour, but a poor blade. Code can, and should change. Just as we should do no harm, we should try leave the code a bit better than we found it.

Sometimes, there are low-risk changes one can do that help improve the codebase: fixing obvious code formatting errors, declaring missing types in doc blocks, replacing deprecated methods with the correct one. Other times, changes are just simply necessary, even though they're difficult to achieve, because if not now, when? Even if asking the developer about it results in a ticket for future work, that's better than just ignoring it.

The flip-side is also true: have they been too much of a good scout, and fixed things that are highly risky, not related to their actual work, and there are no related tests for it? There has to be a good balance to any changes made.

The Reviewer Isn't Always Right

There are many times where I've reviewed code, and believed something to be incorrect. However, after approaching the author, and having a discussion, I've realised that they've thought about the issue, and accounted for something that I've missed. This sometimes means I ask them to add a comment to the code to clarify this, but the point is, it's far better to just ask the author of the code why they did something a certain way, rather than just telling them they've done it wrong.

In addition, having a conversation with the author can demonstrate if they have thought about what they're doing. A common explanation I've heard before is, "I did it that way, because it was like that somewhere else." Yet, they couldn't explain what it was they'd actually replicated.

This demonstrates a lack of understanding, and is a good warning sign that perhaps the code needs closer scrutiny.

Pay Attention To Documentation

This again comes back to the idea that it's not just about the code. The only thing worse than no documentation, is incorrect documentation.

• Is there existing documentation for the work that has been done? If there is, has it been updated? Does it match with the actual implementation?
• Has the developer created new documentation? If they haven't, should they have? If they have, is it correct?
• If it is not the developer's responsibility to update the documentation, have they at least notified whoever is responsible that a change is coming, and provided clear instructions as to what's changed?
  
  

Keep Code Review Iterations Low

Getting code approved should not be viewed as a war of attrition. The reviewer has their own work to do, and the author has other work to focus on. As code reviews drag on, it becomes tempting to spend less and less time on it, to rush and miss things, and to just accept what has been done, saying, "We'll fix it later." Unless exceptionally disciplined, you likely never will.

In order to reduce the back and forth of a code review, it's important to do the following:

• Make sure that comments given are as simple, concise and clear as possible to understand.
• If a comment someone has made is not clear, go and speak with the developer and go through the pull request comments and discuss it before making any changes.
• Make sure you address all the comments someone has raised before sending it back for review. This often happens because developers commit and push code, which can sometimes automatically hide comments on a pull request if code on that line changed. Don't push your code back up until you are sure you've addressed all comments, and it's ready to be reviewed again.
• If code review iterations are increasing, get together and discuss what's going on. Try to identify what's the underlying issue.
  
  

Above All, Think and Communicate

Not all code reviews are equal. Sometimes they're so small and simple that most of what I've mentioned here doesn't apply. The important point is to think carefully about what you're doing, whether you're the author or the reviewer, and to communicate clearly during all stages of the development process.


This article was originally published by Craig Sefton at What makes a good code review.