I discovered FindBugs today and just for fun, I ran it against our current project. Among many, many other things, BugFinder flagged code that created new instances of Boolean rather than use Boolean.TRUE or Boolean.FALSE. This method was tagged as one of the offenders:
public Boolean validateMinLength(String value, int length) {
try {
if (value == null || value.length() == 0) {
return new Boolean(true);
}
if (value != null && value.length() >= length) {
return new Boolean(true);
}
} catch (Throwable e) {
}
return new Boolean(false);
}
I quickly fixed the offending code:
public Boolean validateMinLength(String value, int length) {
try {
if (value == null || value.length() == 0) {
return Boolean.TRUE;
}
if (value != null && value.length() >= length) {
return Boolean.TRUE;
}
} catch (Throwable e) {
}
return Boolean.FALSE;
}
There were still several problems. In fact, it reeked with code smells. A healthy dose of refactoring was in order. Most blatant was the unnecessary and ill-advised try-catch block that silently ignored an unchecked exception. All I can say here is, “huh?”
public Boolean validateMinLength(String value, int length) {
if (value == null || value.length() == 0) {
return Boolean.TRUE;
}
if (value != null && value.length() >= length) {
return Boolean.TRUE;
}
return Boolean.FALSE;
}
Poorly named methods and method parameters are a pet peeve of mine mostly because I use code completion not only to minimize keystrokes but also to jog my memory about the parameters needed by methods. “arg0″ and “arg1″ don’t cut it. “value” was a bit too generic, as was “length.” The method name, “validateMinLength,” implied that the method validated the length parameter instead of checking if the string was a certain length or longer. Eclipse made renaming a breeze:
public Boolean isMinimumLength(String anyString, int minimumLength) {
if (anyString == null || anyString.length() == 0) {
return Boolean.TRUE;
}
if (anyString != null && anyString.length() >= minimumLength) {
return Boolean.TRUE;
}
return Boolean.FALSE;
}
It wasn’t perfect, but the method’s purpose was significantly clearer. An important function of refactoring is separating the intent from the implementation or, in other words, the “what” from the “how.” Without looking at the code, I now had a better idea what the method did but still not how it worked. So let’s take a closer look at the “how.”
Is the intent of the first if statement really to return true when the String parameter is null or an empty string? That’s what the code said, but I was inclined to think it was a bug; logically, null has no length, so how could it meet a minimum length? Not too long ago, I would have added a reassuring comment along the lines of, “// return true if value is null or nill.” I reason that the second if statement actually checked if the string had at least “minimumLength” number of characters but I shouldn’t have to guess. Since I’m a better programmer than I was six months ago 
I extracted new methods to make it more “human readable”:
public Boolean isMinimumLength(String anyString, int minimumLength) {
if (isNullOrNill(anyString)) {
return Boolean.TRUE;
}
if (meetsMinimumLength(anyString, minimumLength) {
return Boolean.TRUE;
}
return Boolean.FALSE;
}
private boolean isNullOrNill(final String anyString) {
return anyString == null || anyString.length() == 0;
}
private boolean meetsMinimumLength(final String anyString, final int minimumLength) {
return anyString != null && anyString.length() >= minimumLength;
}
Now there’s no question that the method really should return true for a null or empty string and when the string has at least minimumLength characters.
Next, using multiple returns violated my rule of one entry point and one exit point for a method. I’ve been in arguments with others who think it’s silly to worry about one exit point, but had the rule been followed, only two Boolean objects would have been instantiated instead of three.
public Boolean isMinimumLength(String anyString, int minimumLength) {
boolean b = false;
if (isNullOrNill(anyString)) {
b = true;
}
if (meetsMinimumLength(anyString, minimumLength) {
b = true;
}
return Boolean.valueOf(b);
}
One last step: while this method was not very complex, there’s no reason it couldn’t be simplified. Also, whenever possible I prefer to avoid using a default return value. Here’s my final refactoring:
public Boolean isMinimumLength(String anyString, int minimumLength) {
boolean b = isNullOrNill(anyString) || meetsMinimumLength(anyString, minimumLength)
return Boolean.valueOf(b);
}
private boolean isNullOrNill(final String anyString) {
return anyString == null || anyString.length() == 0;
}
private boolean meetsMinimumLength(final String anyString, final int minimumLength) {
return anyString != null && anyString.length() >= minimumLength;
}
The public method is readable, even by a non-programmer. It’s faster to gleam the method’s purpose. How the funcionality is implemented is only one method declaration away. This will all come in handy if someone not familiar with the code has to dig in and find out what it does. Lines of code (not counting method signatures) have been reduced from 10 to 4 so bugs are less likely and easier to fix should they occur.
One method down, oh so many to go.
