CodeSOD: A Truly Painful Exchange

Java has a boolean type, and of course it also has a parseBoolean method, which works about how you'd expect. It's worth noting that a string "true" (ignoring capitalization) is the only thing which is considered true, and all other inputs are false. This does mean that you might not always get the results you want, depending on your inputs, so you might need to make your own boolean parser.

Adam H has received the gift of legacy code. In this case, the code was written circa 2002, and the process has been largely untouched since. An outside vendor uploads an Excel spreadsheet to an FTP site. And yes, it must be FTP, as the vendor's tool won't do anything more modern, and it must be Excel because how else do you ship tables of data between two businesses?

The Excel sheet has some columns which are filled with "TRUE" and "FALSE". This means their process needs to parse those values in as booleans. Or does it"

public class BooleanParseUtil { private static final String TRUE = "TRUE"; private static final String FALSE = "FALSE"; private BooleanParseUtil() { //private because class should not be instantiated } public static String parseBoolean(String paramString) { String result = null; if (paramString != null) { String s = paramString.toUpperCase().trim(); if (ParseUtilHelper.isPositive(s)) { result = TRUE; } else if (ParseUtilHelper.isNegative(s)) { result = FALSE; } } else { result = FALSE; } return result; } //snip}

Note the signature of parseBoolean: it takes a string and it returns a string. If we trace through the logic: a null input is false, a not-null input that isPositive is "TRUE", one that isNegative is "FALSE", and anything else returns null. I'm actually pretty sure that's a mistake, and is exactly the kind of thing that happens when you follow the "single return rule"- where each method has only one return statement. This likely is a source of heisenbugs and failed processing runs.

But wait a second, isPositive sure sounds like it means "greater than or equal to zero". But that can't be what it means, right? What are isPositive and isNegative actually doing?

public class ParseUtilHelper { private static final String TRUE = "TRUE"; private static final String FALSE = "FALSE"; private static final Set<String> positiveValues = new HashSet<>( Arrays.asList(TRUE, "YES", "ON", "OK", "ENABLED", "ACTIVE", "CHECKED", "REPORTING", "ON ALL", "ALLOW") ); private static final Set<String> negativeValues = new HashSet<>( Arrays.asList(FALSE, "NO", "OFF", "DISABLED", "INACTIVE", "UNCHECKED", "DO NOT DISPLAY", "NOT REPORTING", "N/A", "NONE", "SCHIMELPFENIG") ); private ParseUtilHelper() { //private constructor because class should not be instantiated } public static boolean isPositive(String v) { return positiveValues.contains(v); } public static boolean isNegative(String v) { return negativeValues.contains(v) || v.contains("DEFERRED"); } //snip}

For starters, we redefine constants that exist over in our BooleanParseUtil, which, I mean, maybe we could use different strings for TRUE and FALSE in this object, because that wouldn't be confusing at all.

But the real takeaway is that we have absolutely ALL of the boolean values. TRUE, YES, OK, DO NOT DISPLAY, and even SCHIMELPFENIG, the falsest of false values. That said, I can't help but think there's one missing.

In truth, this is exactly the sort of code that happens when you have a cross-organization data integration task with no schema. And while I'm sure the end users are quite happy to continue doing this in Excel, the only tool they care about using, there are many, many other possible ways to send that data around. I suppose we should just be happy that the process wasn't built using XML? I'm kidding, of course, even XML would be an improvement.

[Advertisement] ProGet can centralize your organization's software applications and components to provide uniform access to developers and servers. Check it out!