CodeSOD: Yes, No, NO NO NO NO

Mike was doing work for a mobile services provider. He found this in their code:

private static YesNoType toYesNo(String isYes)
{
if (isYes != null)
{
if (isYes.equalsIgnoreCase(“Y”))
{
return YesNoType.fromString(“Yes”);
}
else
{
return YesNoType.fromString(“No”);
}
}
else
{
return YesNoType.fromString(“No”);
}
}

/**
* @param isYes
* @return
*/
private static YesNoType toYesNo(boolean isYes)
{
if (isYes)
{
return YesNoType.fromString(“Yes”);
}
else
{
return YesNoType.fromString(“No”);
}
}

/**
* @param isYes
* @return
*/
private static String fromYesNo(YesNoType isYes)
{
if (isYes != null)
{
String resultStr = isYes.toString();
if (resultStr.equalsIgnoreCase(“Yes”))
{
return (“Yes”);
}
else
{
return (“No”);
}
}
else
{
return (“No”);
}
}

/**
* @param isYes
* @return
*/
private static boolean isYesNo(YesNoType isYes)
{
boolean isBroadbandUser = false;
if (isYes != null && isYes.toString().equalsIgnoreCase(“Yes”))
{
isBroadbandUser = true;
}
return isBroadbandUser;
}

Look, I’m barely even interested in the functions here. They’re all varying degrees of bad, sure, but I really want to focus on YesNoType. I’ve seen people reinvent all sorts of wheels, but to reinvent a boolean in Java of all places, that’s novel. And like every reinvention, it looks like they reinvented it badly: it’s a stringly typed boolean, at least in terms of how it’s used here.

That’s not to say that this code isn’t full of other horrors, but YesNoType is definitely the root of all evil here.

But let’s dig in, anyway, starting with toYesNo(String). If the input string is “Y”, we store a “Yes” in a YesNoType. Otherwise, we store “No”. This means we can set a YesNoType with a “Y”, but when set, it always returns a “Yes”, which means we’re already in a space where we have inconsistent values.

Now, toYesNo(boolean) tells us that we can’t construct a YesNoType directly off a boolean, which seems like a glaring mistake.

fromYesNo, on the other hand, does a wonderful check to see if the YesNoType matches (ignoring case) a “Yes”, so that it can return a “Yes”. It almost seems like we shouldn’t need this at all if we had any sense, but we don’t.

But the real capper is our isYesNo function, which converts our YesNoType into a boolean, aka the thing we should have been using the whole time. Because this code was clearly copy/pasted from somewhere less generic, we have a local variable called isBroadbandUser. A local variable we don’t need, as we could just do this as a single return. Or, better yet, we could have just used a boolean the whole time.

At worst, we could have written a pair of functions bool fromString(String) and String fromBool(boolean) and called it a day. No new type. No weird string comparisons. A boolean is the simplest thing you can have, and to do it so wrong is an achievement.

Source: Read More