Summa

A few weeks ago I ran into a Java problem that took me an embarrassingly long time to solve — I won’t even tell you how long it took me to notice the source of the bug.  It’s a relatively common mistake for starting developers — and clearly it even happens once in a while to experienced guys who aren’t paying attention.  I figured I would detail my problem in the hopes that someone else out there might run across this post and save themselves half an hour or more of combing through their code.

The Puzzle

To simplify the problem significantly, let’s say we’re dealing with a “feature request” system here.  Users can request that certain features be added to the next version of a product.  The system allows the user to enter any feature in an ordinary text field, but tries to be smart about it — if the short description of the feature matches a request already in the system, the application just adds another ‘vote’ to the existing request.  Otherwise, the application creates a new request and gives it one vote.

Either way, the application saves the feature using an ORM tool that is smart enough to update an existing feature request if necessary.

public class FeatureRequestService {
	public void requestFeature(String name, String description,
			Integer userId) {
		Feature feature1 = new Feature();
		findOrCreateFeature(name, description, userId, feature1);
		saveFeature(feature1);
	}
	private void findOrCreateFeature(String name,
			String description, Integer userId, Feature feature2) {
		if(findFeature(name) == null) {
			feature2 = new Feature();
			feature2.setName(name);
			feature2.setDescription(description);
			feature2.setVotes(1);
		} else {
			feature2 = findFeature(name);
			feature2.setVotes(feature2.getVotes() + 1);
		}
	}
	// ...
}

The Effect

No matter what you do, the system always creates a new feature rather than updating an existing feature.  So if you have 123 requests for “Retractable Nozzle,” you’ll see 123 “Retractable Nozzle” features with 1 vote each, instead of 1 with 123 votes.  This is not the desired behavior.

Some of you can immediately see what was going on there.  Of course since this bug seems to manifest itself in the ’save’ function (listing not shown), you might think you’d need to dive into it (when this happened to me, that was my response).  Since I’m not giving you the “save” method’s listing, though, it should be obvious that that isn’t the source of the error.

The Solution

Of course the bug had nothing to do with the “save” function.  The mistake was that method parameters are copies of references.  Here, maybe this illustration will help.

When we call findOrCreateFeature(), a copy of a reference to the Feature object is passed as a parameter.  When that new reference is modified by making it point at a new object (feature2 = new Feature()), it no longer points at the original Feature object.  Any changes called on feature2 are applied to this new Feature object. When findOrCreateFeature() ends, feature2 goes out of scope and its new Feature object is destroyed.

Advice

The rule of thumb is: Don’t reassign method parameters.  In fact, this is the main reason why most code style guides and correctness checkers direct you to make all your method parameters final.  If findOrCreateFeature()’s signature had been…

private void findOrCreateFeature(final String name, final String description, final Integer userId, final Feature feature2)

… then my foolish attempt to reassign feature2 would have caused a compiler error, and I would have saved myself at least half an hour.

You can still hand off these sorts of duties to private methods of course; you just have to be a lot more careful about how you handle the objects that you’re passing around.