A short guide to code review

I’ve admitted before that I’m no great fan of code review. Then again, I know of no other process for catching the most painful problems that would catch up with you sooner or later. Over the past few years I’ve worked out a list of areas worth looking at every time and a few rules I try to follow.

Code smells and object-oriented design

The general rule is that well-designed object-oriented code is less prone to problems. I pay special attention to classes that act like the Central Brain of Humanity. You can spot them by the piles of procedural code. The Business Facade or Service Facade pattern doesn’t mean cramming all the logic into one class, as too many programmers seem to think.

While we’re on code smells, it’s often enough to look at the biggest classes, those with the biggest tests or those with no tests at all. Code that’s hard to test generally has design problems.

During code review I also look at whether the code is defensive enough. What really needs to stay public? Why is something missing final? Anything that smells of duplication has to go. So does commented-out code. If you need an example, put it in Javadoc. When I say code, I mean the surrounding infrastructure too, including Maven and Spring.

Synchronization and thread safety

One of the biggest sources of trouble is code that fails to maintain memory consistency under multithreaded access. If you’re writing a web application or handling messages today, you’re exposed to that risk whether you realize it or not.

Programmers understand how to use locks for mutual exclusion. The harder thing to remember is that synchronization primitives also guarantee that data is visible across threads. It matters when you pass objects between contexts handled by different threads. And it’s only going to get trickier with features like asynchronous processing in Servlet 3.0.

Focus on objects that hold state. Check the behavior of third-party libraries too, and whether they were designed for the kind of multithreaded use your setup requires. In general, you’ll have the fewest problems if you build stateless services and pass state around in immutable objects.

Locks, deadlocks from calling yourself, and other dirty tricks

Locks can cause plenty of trouble, whether they’re in memory or in a database. The first thing to check is whether the lock can actually work. A lock is often fine locally in a single running JVM. If you deploy to a cluster, check that it works across the cluster too.

If you need a lock that actually works in a cluster, it will probably be backed by a database. Carefully go through how the lock gets released, ideally with an example of a JVM instance that crashes. While we’re on locks, code that depends in some way on initialization order can cause similar problems, especially in a cluster.

Pooled resources are a subject of their own, whether it’s database connections, HTTP threads or something else. Check that a single call never allocates more than one resource. If it does, a deadlock is likely sooner or later. All it takes is for the number of threads accessing the pool to equal or exceed the number of available resources.

Releasing and allocating resources

When you allocate resources you have to return explicitly, such as memory or file descriptors, check that this happens in try/finally blocks. A particularly nasty problem is assigning more than once to a variable that points to a resource you have to return. A good rule is to check that these variables are final. That reduces the risk of this sort of problem.

When allocating resources, check how they’ll be released. Pay close attention to anything tied to a transaction. If a transaction can run for a long time, check how allocated resources get released while it’s running, if they do at all. Transactions can allocate memory or lock other resources, such as database rows and tables, as a side effect.

A few observations

Expecting a review to catch every problem someone introduced over N days of work is unrealistic. Code review should help us get rid of the worst mistakes. Anything else is a bonus. If you don’t plan to fix problems right away or file a defect you’ll come back to later, the review has little value except for sharing knowledge about the code.

Look at other details too. How long do the tests take? How much longer will the application server take to start? How much more memory will the code use? How long will it take to build? Questions like these help you see the full picture during a review.