Code Reviews–What are we looking for?

A few years back I wrote about the issue of when to perform code reviews. Code reviews which happen late in the development cycle can be a waste of time, since it may be too late to act on the findings. However, there is another, much bigger, problem with code reviews, and that is the issue of what are we supposed to be looking for in the first place?

At first this might seem to be a non-issue. After all, the reviewer is simply looking for problems of any sort. But the trouble is that there are so many different types of problem a piece of code might contain that it would take a very disciplined reviewer to consciously check for each one. It would also take a very long time. The reality is that most code reviewers are only checking for a small subset of the potential issues.

So what sorts of things ought we to be looking for?

 

Code Standards

I put this one first as it is the one thing that almost all code reviewers seem to be good at. If you break the whitespace rules, variable naming conventions, or miss out a function or class comment, you can be sure the code reviewer will point this out.

However, these are things that a good tool such as ReSharper ought to be finding for you. Human code reviewers ought to be focusing their attention on more significant problems.

 

Language Paradigms

Other things an experienced developer will be quick to pick up on are when you are using the language in a sub-optimal way. In C#, this includes things like correct use of using statements and the dispose pattern, using LINQ where appropriate, knowing when to use IEnumerable and when to use an array, and making types immutable where appropriate.

Observations like this are particularly useful to help train junior developers to improve the way. But again, many of these issues could be discovered by a good code analysis tool.

 

Maintainability

In a large system, keeping code maintainable is as critical as keeping it bug free. So a code review should look for problems such as high code complexity. I almost always point out a few places in which the code I review could be made “cleaner” by refactoring into shorter methods with descriptive names.

Uncle Bob Martin’s “SOLID” acronym is also a helpful guide. Are there bloated classes with too many responsibilities? Are we constantly making changes to the same class because it isn’t “open to extensibility”? Are we violating the “Liskov Substitution Principle” by checking if an object is an instance of a specific derived type?

Noticing maintainability problems at code review time, such as tight or hidden coupling through the use of singletons, may require some substantial re-work of the feature, but it will be much less painful to do it while it is still fresh in the developer’s mind rather than having to sort the mess out later on in the project lifetime.

 

Project Conventions

Every software project will have some kind of organization and structure, and in an ideal world, all developers are aware of this and make their changes in the “right” place and the right way. However, with large projects, it can be possible for developers to be unaware of the infrastructure that is in place. For example, do they ignore the error reporting mechanism in favour of re-inventing their own? Do they know about the IoC container and how to use it? Did they keep the business logic out of the view code?

For issues like this to be picked up, the code reviewer needs to be someone familiar with the original design of the code being modified. It may be necessary on some occasions for the code reviewer to ask the developer not to copy an existing convention because it has been discovered to be problematic.

 

Finding Bugs

Most discussions of code reviews assume that finding bugs is the main purpose, but from the list of items I’ve already mentioned it is not hard to see how we could actually forget to look for them.

This involves visually inspecting the code looking for things like potential null reference exceptions, or off by one errors. A lot of it is about checking whether the right thing happens when errors are encountered.

The trouble is that if the code is highly complex or badly structured, then it may be close to impossible to find bugs with a visual inspection. This is why the “maintainability” part of the code review is so important. Giray Özil sums this problem up brilliantly:

“Ask a programmer to review 10 lines of code, he'll find 10 issues. Ask him to do 500 lines and he'll say it looks good.”

 

Meeting Requirements

If the code reviewer doesn’t have a firm grasp of the actual requirements that the coder was trying to implement, it is quite possible that code that simply doesn’t meet the requirements gets through the code review. The more understanding a code reviewer has of the actual business problem this code is trying to solve, the more likely they are to spot flaws (or gaping holes) in its implementation.

 

User Interface

Now we move onto some areas for review that relate to specific types of code. The first is user interface code. This adds a whole host of things for a reviewer to check such as correct layout of controls, correct use of colour, all localisable text read from resource files, etc. There will likely be some established conventions in place for user interface, and for a professional looking project, you need to ensure they are adhered to.

There is the also need to review user interfaces for usability. Developers are notoriously bad at designing good UIs, so a separate review with a UX expert might be appropriate.

 

Threading

Multi-threaded code is notoriously hard to get right. Are there potential race conditions, dead-locks, or resources that should be protected with a lock? It is important that a code reviewer is aware of which parts of the code might be used in a multi-threaded scenario, and give special attention to those parts. Again, the code needs to be as simple as possible. A 4000 line class that could be called on multiple different threads from dozens of different places will be close to impossible to verify for thread safety.

 

Testing

A good code reviewer should be asking what has been done to test the code under review, including both manual and automated tests. If there are unit tests, they too ought to be code reviewed for correctness and maintainability. Sometimes the best thing a code reviewer can do is suggest additional ways in which the code ought to be tested (or made more testable – the preference should always be for automated tests where possible).

 

Security

One of the most commonly overlooked concerns in code reviews is security. There are the obvious things like checking the code isn’t vulnerable to a SQL injection attack, or that the passwords aren’t stored in plaintext, or that the admin-level APIs can only actually be run by someone with the correct privileges. But there are countless subtle tricks that hackers have up their sleeves, and really your code will need regular security reviews by experts.

 

Performance

This is another commonly overlooked concern. Of course, much of our code doesn’t need to be aggressively performance tuned, but that doesn’t mean we can get away without thinking about performance. This requires knowledge of what sort of load the system will be under in “real-world” customer environments.

 

Compatibility

Another big issue particularly with large enterprise systems that the code may need to run on all kinds of different operating systems, or against different databases. It might need to be able to load serialized items from previous versions of the software, or cope with upgrading from the previous version of the database. If these problems are not spotted in code review, they can cause significant disruption in the field as customers find that the new version of the software doesn’t work in their environment.

Where possible, automated tests should be guarding against compatibility breakages, but it is always worth considering these issues in a code review.

 

Merging

If you are in the situation where you need to actively develop and maintain multiple versions of your product, then a code reviewer needs to consider the impact of these changes on future merges. As nice as it might be to completely reformat all the code, rename all the variables and reorganize all the code into new locations, you may also be making it completely impossible for any future merges to succeed.

 

Summary

Looking at the list above you might start to think that a code reviewer has no hope of properly covering all the bases. For this reason I think it is important that we clarify what a particular code review is intended to find. There may need to be special security, performance or UI focused reviews. I also think we should be using automated tools wherever possible to find as many of these issues for us.

I’d love to hear your thoughts on this topic. Which of these areas ought a code review to focus on? Have I missed any areas?

Crowd-Sourced Code Reviews

In a post yesterday, I bemoaned the lack of a clean syntax in NUnit for testing whether events fired from an object under test, and set to work at writing my own helper methods. I posted on the NUnit discussion group, to see whether there was a better way. And there was. A really obvious way. Here’s how my tests could have been written without the need for an Assert.Raises function, or for any code outside the test method:

[Test]
public void TestRaisesClosedEvent_Improved()
{
    Blah blah = new Blah();
    EventArgs savedEventArgs = null;
    blah.Closed += (sender, args) => savedEventArgs = args;
    blah.RaiseClosed(1);
    Assert.IsNotNull(savedEventArgs);
}

[Test]
public void TestCanCheckRaisesMany_Improved()
{
    Blah blah = new Blah();
    var savedEvents = new List<openedeventargs>();
    blah.Opened += (sender, args) => savedEvents.Add(args);
    blah.RaiseOpened(5);
    Assert.AreEqual(5, savedEvents.Count);
    Assert.AreEqual("Message 3", savedEvents[2].Message);
}

Of course, I was kicking myself for missing such an obvious solution, but it left me pondering how often this happens without me realising it.
Earlier today Clint Rutkas tweeted:

i feel like i'm doing such bad things in WPF right now

… and I know exactly how he feels. In a couple of WPF applications I am working on, I am trying to use the MVVM pattern. It often ends up with me writing code that might be a really cool piece of lateral thinking, or it might be a pointlessly overcomplicated hack. For an example, here’s an  attached dependency property (usage here) I created to let me bind to Storyboard completed events.
What I need is another person to look over my shoulder and tell me whether I am missing the obvious, unaware of an API, or ignorant of a best practice. But all too often, that person doesn’t exist.
In a commercial environment, hopefully there is at least some kind of provision for code reviews to take place, or maybe even pair programming. But what about the lone programmer working on open source projects?
Maybe we need some way to “crowd-source” code reviews. Some way of getting lots of eyes on your source code, even if it is only for a couple of minutes, and an easy way of getting hold of that feedback. A bit like fivesecondtest but where you help out an open source developer by code reviewing a changeset. I’ve asked for this as a feature on CodePlex.
What do you think? Could it work? How do you make sure you’re not missing the obvious in the code you write?

When to Review Code

I am assuming I can take it for granted that most developers acknowledge the value of performing code reviews. All the companies I have worked for have had some kind of policy that states a code reviews must take place, but when the code review takes place can vary dramatically. While I was writing this post, I came across a couple of helpful Stack Overflow questions which have more insights on the when a code review should take place. (When to review code - before or after checkin to main and When is the most effective time to do code reviews)

I'll discuss three possible times to perform a code review:

Post Check-in

I think this is the most common time for code reviews to take place. The developer checks in their code, then arranges for it to be reviewed.

Advantages:

• The developer has actually "finished" coding, meaning that issues found really are issues as opposed to "oh yes I was going to do that later".
• The developer is not held up waiting around for a reviewer - they can get their code checked in and start work on the next task.
• Source Control systems typically have tools which make it very easy to highlight just the changes that have been made in a particular check-in. This is very convenient for reviewing bug fixes.

Disadvantages:

• Unreviewed code can get into into main branch, increasing the chances of an inadvertent show-stopper bug.
• There is potential for the review to be put on the back burner and forgotten as there is nothing being held up by it.
• Management may view this feature as "complete" and thus resist allocating further time on it if the review recommends some refactoring.
• The checked in code may have already gone through a manual testing cycle which is a strong disincentive to touch the code even to implement minor enhancement suggestions (as they invalidate the tests).
• The developer is likely working on a new task when the code review takes place, meaning that it is no longer fresh in their mind, and (depending on the way source control is used) can easily end up mixing code review enhancements with new code for an unrelated feature in a single check-in.

It is interesting how the use of branches in source control can lessen the impact of many of the points I have made here (both advantages and disadvantages).

Pre-Checkin

One company I worked for had a policy of pre-checkin code reviews. For small changes, another developer came and looked over your shoulder while you talked them through the changes, and for larger features, you emailed the code across and awaited the response.

Advantages

• The code is still fresh in the developer's mind as they have not yet moved on.
• It is clear to the manager that we are not yet "finished" until the code review, resulting code modifications and re-testing have taken place.
• The fact that the code is still checked out means that it is very easy to make changes or add TODOs right in the code during the review, which results in less suggestions getting forgotten.
• The code can be checked in with a comment detailing who reviewed it - share the blame with your reviewer if it contains a show-stopper bug!

Disadvantages

• Though the code is "ready" for check-in, it may need to be merged again before the actual check-in, which can sometimes result in significant changes.
• It can make it harder for the reviewer to view the changes offline.
• A strict no check-in without code review policy can result in people making fewer check-ins, and batching too much up into one check-in.
• Can hold up the developer from starting new work if the reviewer is not available.

Got it working

There is one other potential time for a code review. In Clean Code, Bob Martin talks about the "separation of concerns" developers tend to employ while coding. That is, first we "make it work", and then we "make it clean". We have got the main task complete, but things like error handling, comments, removing code smells etc have yet to take place. What if a code review took place before the "make it clean" phase?

Advantages

• The developer does not need to feel so defensive about their code, as they have not completely finished.
• Its not too late to make architectural changes, and maybe even refactor other areas of the codebase to allow the new code to "fit" better.
• Because there is still time allocated to the task, it can effectively serve as a second design review, with an opportunity to make some changes to the design with the benefit of hindsight.
• Could be done as a paired programming exercise, experimenting with cleaning up the code structure and writing some unit tests.

Disadvantages

• Wasting time looking at issues that were going to be fixed anyway.
• May not be appropriate for small changes, and with large pieces of development, may depend heavily on whether the coder follows many small iterations of "make it work, make it clean", or a few large iterations.

Final thoughts

There are two factors that greatly affect the choice of when to code review. The first is your use of Source Control. If individual developers and development teams are able to check work into sub-branches, before merging into the main branch, then many of the disadvantages associated with a post-checkin code review go away. In fact, you can get the best of both worlds by code reviewing after checking in to the feature branch, but before merging into the main branch.

Second is the scope of the change. A code review of a minor feature or bugfix can happen late on in the development process as the issues found are not likely to involve major rearchitecture. But if you are developing a whole new major component of a system, failing to review all the code until the developer has "finished" invariably results in a list of suggestions that simply cannot be implemented due to time constraints.