What Makes a Good Code Review?

Code Reviews are integral to building quality, robust software, and at Eagle Eye, they are an important part of our software development lifecycle, helping our engineers ensure they release better code, and also helping them learn and grow. What follows is a set of principles and guidelines we encourage our Engineers to follow whenever they find themselves reviewing someone's code, or someone is reviewing theirs.

Start With Trust

Code reviews are a collaboration between the code author, and the reviewer, and the key is to trust each other. Both have the best of intentions, namely to build software that solves a problem. Code reviews are not there to judge or berate people, or show how smart someone is. Criticism should not be taken personally, and be as constructive as possible. Code reviews are opportunities to learn from each other, and improve quality. Treat each other with respect.

Code Reviews Begin With The Author

The reviewer is giving up their time to help ensure the quality of what has been done, not to fix the author’s code. The author should do all they 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 begins before a line of code is even written.

The author should make sure that they know what they're about to build, and think carefully about how they plan on approaching it. 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 engineers 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.

Keep Your Code Review Small

The more code an author creates or changes, the longer the code review will take. The author has the benefit of learning as they build, while a code reviewer only sees the finished product. This is why reviewing small pieces of code is often faster and safer. Try and keep the scope of changes as focused as possible.

Give A Good Subject And Description

Assuming you're using GitHub, the author should give their pull request a good title that describes succinctly what has been done. Typically, I give my pull requests a title in the format of 'Ticket: Subject'. Since we use Jira, we use the Jira ticket, but if I'm using GitHub issues, I will use "Issue 123".

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 the 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 the author’s own understanding of what they’ve done: if the author can't describe what has been done clearly and simply, this can be a warning sign that maybe they’ve not fully understood the ticket, or the code itself.

Be Your Own Code Reviewer

Before the author even assigns the code review to anyone, they should review it themselves. 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 fulfills 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, and 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, and 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 to do 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.
• 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.