Code Review Checklist: Stop Producing Bugs Now!

Interested in a great code review checklist to be able to stop producing bugs? 

You’ve come to the right place.

A good code review checklist will allow you to build a great bug-free product that makes lots of money.  In this blog post, aimed at CTOs and software development professionals, we look at what you can do to reduce the number of bugs in your software products.

I‘ve been a developer, both contract and permanent, long enough that I wish I could utter a magic phrase and end development bugs forever. It‘s not that easy, unfortunately; it‘s not rocket science either.

If you had asked me about how to reduce bugs back when I first started programming I‘d have thrown my arms up in the air in exasperation at the impossible task you‘ve just asked me to achieve.

Move on a few years (ok, maybe almost a few decades), we have built-in real source control integration that isn‘t visual source safe (probably the worst source control system ever); automated unit, integration, and user interface testing; code analysis; the list goes on.

There are lots of free, open-source, and commercially licensed automated code review tools– the choice is yours. A word to the wise though, you can spend an absolute fortune on the latest tools but if your practices and processes don‘t hold up all you‘ll achieve is headaches!

So how do you make sure your practices and processes hold up against scrutiny? You review the code. A code review checklist is a must for any project if you want it to succeed.

Building a code review checklist

When I run a code review I try to make sure they are all treated equally; to do so, I built a checklist to remind me, and the teams I work with, what needs to be checked (a rough and ready sample is shown below)

Scope

Imagine the code we are reviewing is a piece of paper – and a potential bug/issue or misinterpretation is a grain of sand. Would it be easier to find that grain of sand on a postcard-sized piece of paper or on a piece of paper the size of a house?

When you are carrying out a review you should be asking yourself “Does this change serve a single purpose?” if you can answer yes, then continue with the review.

Get a complimentary discovery call and a free ballpark estimate for your project

Get Started

Trusted by 100x of startups and companies like

If not then that leads to a bigger question – “Has the solution been correctly designed?” or worse “Has the solution misinterpreted the requirements?”

As a reviewer, you need to be objective about the code you are reviewing – if you are struggling to keep track of where you are then you can‘t devote your full attention to the review.

Why is this important? We all like to think that we are intelligent, and able to multitask – after all, we program computers for a living. Studies have proven people are inherently bad at multitasking.

Every time you switch to a new task you decrease your productivity, both the cognitive load of learning the new task, as well as the inability to fully let go of the previous task, occupy your thoughts.

Keeping your submission and review focused on a single purpose helps maintain focus on the task at hand – checking the code does what it is meant to do.

Scale

Remember that piece of paper with the grain of sand on it I just talked about? What would happen if instead of one piece of paper we scaled that up so you had to review 50 pieces to find that one grain of sand? Finding that grain of sand (the bug) would be orders of magnitudes harder.

Scale is another important aspect of both the submission and the review. Make sure only the required objects are included in the review – you may well have to deal with 500 classes due to a major refactoring exercise or a drastic redesign – but often this can be broken down into more reasonable chunks.

As a reviewer, it is your duty to ensure that the specific collection of objects you are being asked to review belong together. The cognitive load I discussed in the previous section gets worse as the complexity goes up!

Design Principles

A key factor of the review, that goes hand in hand with the Scope and Scale from the previous sections, is Architecture. Is the solution well architected? Does it make use of the various principles that your preferred language holds key to its implementation? In my experience these are:

Separation of Concerns – each module or class should be responsible for a single part of the functionality of the application.

This DOES NOT mean you need one class for each function. The intention is to ensure that you don‘t have “god” objects – i.e. a single class that does absolutely everything and therefore more than likely quite sensitive to change.

Ask yourself “are helper objects tightly coupled?”, “Do classes manipulate instances of another class?” if the answer is yes, there may be an issue here.

Keep it simple, stupid! (KISS) – Keep the solution, its modules/classes, and functions simple. Overly complex code is harder to debug and harder to ensure all branching logic can be tested sufficiently.

Don‘t Repeat Yourself (DRY) – If you find yourself reading essentially the same code over and over, think about whether the code could be restructured so that the methods can be REUSED. Code reuse reduces complexity (normally) and helps in testability.

Architecture – if the solution is an MVC solution does it adhere to those principles? Are there any deviations? If there are, why? The answer here isn‘t that a developer should be doggedly sticking to a given architecture – but rather that any deviation is thought out and reasoned.

Syntax and Structure

Most organizations have some sort of coding standard; whether this is informal or formal can depend on a whole gamut of factors. Normally though, the coding standard has evolved through time based on the changing fashions and tastes of the developers involved in building and maintaining the code.

Why is a coding standard important? It allows all the developers involved in the process to work with a standard set of rules. If we all work on an even footing then we can ensure that the code review process of finding bugs and ensuring code quality is simpler.

So what does a coding standard guide look like? Let‘s have a look at what Microsoft thinks should go into one.

At its simplest, there should be some rules about how things should be named, and how code structures should be typed.

foreach (Customer CUST in Customers)
DoSomething(CUST);
doSomethingElse();

In the above code snippet there are a few things that may not meet normal, accepted, standards:

• Local Variables – normally lowercase or camelCase
• Explicitly typed variables – use implied typing, this makes your code more flexible. In this case, imagine we change the Customer collection from List to List so that it can be used against individuals or organizations. You now need to refactor all your code, using var instead would mean you only need to refactor code that breaks (see Unit Tests).
• Loops with a defined start and end – loops should always enclose their body in a start and end (curly braces in C# or JavaScripts case). This ensures that everyone knows what is scoped in the loop and what isn‘t. In the above example, a cursory glance leads you to believe two statements should be executed in the loop; closer inspection only DoSomething(CUST) is within the loop.
• Consistent Naming – you can see mixed casing for the methods being called. shows As long as the methods in the relevant classes and objects are named in the same way your code will compile but it adds visual clutter.

The above code readability would be much more better if it was represented like this

foreach(var cust in Customers)
{
DoSomething(cust);
}
DoSomethingElse();
It‘s now much clearer what is in the loop and what is outside the loop. Visually the code is more balanced and includes hints as to what is a method and what is a variable or property.

Your inspection should include checking the reviewed code meets the standards you set in your organization but don‘t worry – you‘re not going to be a glorified code formatting expert there are tools out there that can help!

I‘ve used StyleCop for many years now, both in CI environments and those without. It comes in various guises, it can be integrated into Visual Studio if you want it to be; you can also integrate it with ReSharper and your Automated Builds if you want to.

Compiler Warnings and Errors

Great, so now we have clean and tidy code, that‘s split into simple, easy to understand chunks. Now what? Does the code even compile?

If there are any errors in the compilation then something is wrong; the code is obviously broken. We are all different; we all have our own idiosyncrasies and preferences for tools, file locations, and structures.

So it stands to reason what may compile on my machine might not compile on another developer‘s computer. Why though? Is it a missing reference?

An environmental issue or dependency that doesn‘t exist? What would happen if a customer tried to run the code compiled in my development environment on their own device?

This is why a clean, automated build environment is key. It helps remove the environmental factors; ensuring that all builds have all the components they require included in them.

If the code doesn‘t compile, unless you are specifically reviewing the reason for the compilation error; fail the review there and then, continue with the rest of the code reviewing though as it makes no sense to give up and leave other potential issues lurking.

Ok, so we got rid of the compile errors? What about warnings? Warnings are there for a reason, they hint at where a bug may be coded into your application in the future.

Fix it or ignore it – that decision is up to you, and your coding standards – if you ignore it though at least ensure why you are ignoring it is documented somewhere and suppress the warnings.

Personally, I feel suppressing warnings is a bad idea, but I would recommend code-based suppression of the warnings as that way it’s version controlled and not dependent on your specific development environment.

Microsoft has detailed documentation on suppressing warnings in code (other IDEs have their own way of doing things); essentially you have to prefix the method or class that contains the violation with an attribute something like this

[System.Diagnostics.CodeAnalysis.SuppressMessage(“Microsoft.Usage”, “CA1801:ReviewUnusedParameters”, MessageId = “args”)]

Good luck, and remember not to sweep too many warnings under the rug!

Unit Tests

Testing – who needs it right? “I‘m a developer – I have experience in writing code, testing takes too much time and I‘ve got plenty more to get on with”.

I‘ve been there; read the book; and got the t-shirt several times now. Let‘s assume we are all perfect coders and that our code doesn‘t contain any bugs. Does it actually do what was intended? How do we prove that?

Unit testing would tell you that what you coded and how it behaves matches what the design says your code should do. You should be testing expected behavior and edge cases.

If you‘ve written an age validation routine you should be testing the upper and lower bounds, median values as well as values that should never be allowed. This will ensure your code meets requirements and is robust enough to be used in production.

If you can automate those tests even better – you can test more scenarios in less time that way and be sure that each and every time you make a change that you haven‘t broken your own new code or existing code that was written by a team member.

Not all code needs unit tested – you only need to test the public faces of your code – public classes, methods, and properties. You don‘t need to worry about the private methods normally as there should be no code that exists unless it‘s in use (we will discuss this later).

Your review should include checking to see that the unit testing is sufficiently rigorous – don‘t be afraid to point out where a unit test may be weak or could be extended by adding more scenarios.

Code Coverage

As a code reviewer, you need to make sure that there are sufficient tests written to ensure the functionality of the code is tested in the usual use cases – unless it‘s a very simple algorithm there‘s no way that you could test every possible eventuality. So when reviewing code set a sufficiently high bar for acceptance. 100% code coverage doesn‘t mean all possible paths/branches/states are tested. It just means each public method or class is tested at least once.

While nothing is stopping you achieving 100% coverage – it is more important to ensure the Unit Tests themselves are rigorous enough to pass more than just the ideal scenario. This is why I rank checking code coverage lower than the unit tests themselves.

Measuring Code Coverage

Visual Studio has built-in Code Coverage tools – but only if you use the Enterprise Edition;

Personally, I prefer DotCover from JetBrains as part of the Resharper Ultimate package. With DotCover, you get an inline visualization of what is covered by a Unit Test and what isn‘t as well as an early indication of when a test is broken.

If your budget doesn‘t stretch to VS Enterprise or Resharper then there are free tools if you‘re willing to put in a little extra work that will give you something like this as output:

OpenCover is an open-source tool but you‘ll need to install it and ReportGenerator to get the best out of it

Install-Package OpenCover -Version 4.6.519.
Install-Package ReportGenerator -Version 3.0.1.

You can find out how to use OpenCover here and ReportGenerator here – their use is a bit beyond the scope of this blog but if you‘re interested you could always suggest that as a topic for the future.

Tools

I‘ve highlighted several tools that I use daily in my line of work – if you haven‘t heard of them or want to read more about them I‘ve provided links below for you to use

TFS Code Reviews: https://docs.microsoft.com/en-us/vsts/tfvc/get-code-reviewed-vs
Unit Testing Frameworks
Microsoft Visual Studio Unit Testing: https://msdn.microsoft.com/en-us/library/microsoft.visualstudio.testtools.unittesting.aspx
JavaScript Unit Testing using QUnit: https://qunitjs.com/
ReSharper: https://www.jetbrains.com/resharper/
StyleCop: https://github.com/StyleCop/StyleCop

Code ownership and depersonalization

Finally, a little aside to both the Reviewer and the Reviewed. A Code Review should not be a personal attack on one or the other; it shouldn‘t cause anger or animosity between team members.

It is a tool, like many others that we use in our working lives, to assist in raising the standard of output from a good solution to a brilliant solution.

That being said, there is also no harm in pointing out or appreciating good code solutions – I take pride in my solutions, especially when I feel they are elegant or simplify an otherwise complex problem. I think most developers do.

The added benefit of a code review is that it presents a formalized opportunity to share knowledge – I can‘t tell you how many times in the past that I‘ve been handed a bug list, and told to fix it all while having never seen the code or the requirements before.

Your job will ultimately be made easier by having formalized code reviews.

Final thoughts on using a code review checklist

The process I‘ve discussed; using the tools, techniques, and measures helps build a fuller picture of what is required in a code review checklist. Now that we have a better understanding it‘s a really simple process to put it all together into a checklist to help guide you through the next review you carry out.

This isn‘t designed to be a comprehensive list but rather a starting point for you to customize so that it suits your team, projects, and organization.

I‘ve highlighted some of the things I believe are key to a successful code review. It‘s not the only way to carry out a code review, there may be some things you want to add to or remove from your own code review checklist.

The Important thing is to review code – review your own code, and review others. The fact you are reviewing it will make the code base better, as well as spread knowledge in the team. Let us know if you have implemented these or if you think I‘ve missed anything.

Frequently Asked Questions on code review checklist