refactoring java to smell nice

Tue, Apr 19, 2011

Have you ever written a method in a Java class and then wished you could return two things from it? Have you also noticed a strange odour coming from your machine at the same time? That’s code smell! Any time you think you need to return two completely unrelated values from the same function means the method is trying to do too much. Let me explain with an example from the other day.

I’m in the latter stages of developing and testing a distributed provisioning system that I wrote in Java and has Apache Camel and ActiveMQ at its heart. One system I need to provision is Novell eDirectory and the messaging code basically boils down to processing an incoming User message and if it could create the account in eDirectory it sends a success message to the hub and also returns a a reusable MatrixConnection object which is passed in, in subsequent calls, for sending more success messages.

public MatrixConnection processUser(User user, MatrixConnection previousConnection) {
  createUser(user);
  return sendSuccessMessage(previousConnection);
}
On top of this, processUser handles storing a failed account message in the backing store for a later retry, so it hides everything from the caller. All the caller gets back is a MatrixConnection to send back in. This was fine until I added support for processing the backing store and retrying failed accounts. The caller in this case needed to know whether the account was successful so it could remove the failed account from the backing store. So all of a sudden, processUser needed to return two things. A reusable connection and a boolean. Smell that stench!

There are two common ways to deal with this pong. One is to create a new inner class, visible only to the members and methods of the containing class and return that:

class ProcessUserResult {
  MatrixConnection connection;
  boolean result;
}
The BackingStore class does just this. It needs to return information for a StoredMessage. i.e. it needs to return multiple things from one method. It does this by using the above technique. It returns a StoredMessage object. But the difference is, StoredMessage groups related information, whereas ProcessUserResult doesn’t. It’s just a bin into which more and more things can be thrown and as we all know, bins smell. Indeed ProcessUserResult doesn’t smell any nicer than the original fumes emanating from the system.

The other way to cover up an odious odour is to change the parameters passed in. This is common in C, C++ and Objective-C using pointers and references. However, there’s a visual clue in these languages that alerts the coder that trouble may be brewing. For instance, in those languages we could rewrite the method signature to be:

public boolean processUser(User user, MatrixConnection* previousConnection)
and pass back the reusable connection directly in the previousConnection parameter. The method signature at least hints that previousConnection will change. Look at the same technique in Java though:
public boolean processUser(User user, MatrixConnection previousConnection)
The method signature hasn’t changed in the parameter department at all. There’s nothing there to say that previousConnection is going to be changed by the method. This is bad form and you should never change parameters in the method that uses them. Ever!

So now I’ll qualify what I said earlier. If you need to return multiple unrelated things from a method then the method is trying to do too much. And in that case you should refactor in the Ruby way. Ruby borrows its class design philosophy from unix, where the tradition is lots of small programs doing one thing that can be piped together to form systems. For example, there’s no unix countlines program. Instead you pipe specialised programs together:

cat smell.txt | wc -l
In much the same way, a Ruby class is designed such that each method is small, lightweight and does one thing. You should aim to follow this philosophy in your Java code. With this in mind I refactored to put all the success message sending code into another method that accepted and returned a connection object:
public MatrixConnection sendSuccessMessage(MatrixConnection previousConnection)
and a similarly focussed method to provision the account:
public boolean processUser(User user)
Suddenly the code smelt of roses and lavender and small birds came and chirrupped on my window sill!

You might at this point wonder why I didn’t just store the previous connection object as a class variable? It has to support threaded access by ActiveMQ and that’s a whole different kettle of poo…

comments powered by Disqus