Boy scout, light my fire

I’m a boy scout coder, I admit it freely and it’s also something I’m proud to say. Never heard of it? Okay, let me explain. The boy scout coder commits code in better shape with every commit, that’s the rule. Always improve something before you commit your changes, or do a separate commit for the quality enhancement, but always improve tiny imperfections.

In a previous entry, I wrote about the isEmpty() method and the proper way of using it. Today, I got the chance to show you a proper example and also clean up some code smell. When implementing some new functionality, I came across this ugly looking beast:

private static String arrayToString(Set<String> services) {
	if (services.size() > 0) {
		StringBuffer sb = new StringBuffer();
		for (String service : services) {
			sb.append(service);
			sb.append(",");
		}
		return sb.substring(0, sb.length() - 1);
	} else {
		return "";
	}
}

What’s wrong with it? #

It has a number of issues actually, first and foremost of course, it’s not using the isEmpty() method to handle the exception case, it uses the size() method to check if we have data and then act upon it. This of course, is completely not the way to do it. Secondly, it has multiple returns, making it harder to follow what the code actually does. A method should have a single return, but sometimes it is hard to do. In this case, it is due to a lazy developer, time constraints or just plain ignorance. The third issue might be a bit trickier to see, but it is using a StringBuffer, which is a Synchronized object. If you need thread safe Objects, this would be the way to go, but in this instance, we don’t actually need it and using a StringBuilder would be a lot faster.

Refactoring #

As the boy scout I am, I refactored the method into:

private static String arrayToString(Set<String> services) {
	StringBuilder returnString = new StringBuilder();

	if (services.isEmpty()) {
		// Prevent StringIndexOutOfBoundsException in return
		returnString.append(",");
	}

	for (String service : services) {
		returnString.append(service).append(",");
	}

	return returnString.toString().substring(0, returnString.length() - 1);
}

Okay, so now we have a single return, we don’t need to look for some return further up in the code, it’s only found last in the method, where you would expect it to be. Secondly, I use the isEmpty() method to check if it’s empty, because in this case I have a return that will throw an exception in case I don’t handle it. I have also refactored the method into using a StringBuilder instead of a StringBuffer.