CodeSOD: Unification of Strings

As a general rule of thumb, when you see a class called StringConverter you know something is going to be wrong in there. That's at least what Erik thought when examining a bug in a totally different section of string handling code that just happened to depend on StringConverter.

StringConverter might sound like some sort of utility belt class with a huge pile of methods in it, but no- it's only got two. So we should take a look at both.

Let's start with the method called Escape.

 public static string Escape(string s) { if (string.IsNullOrEmpty(s)) return s; StringBuilder sb = new StringBuilder(s); sb.Replace("A", "Ae"); sb.Replace("O", "Oe"); sb.Replace("U", "Ue"); sb.Replace("a", "ae"); sb.Replace("o", "oe"); sb.Replace("u", "ue"); sb.Replace("", "ss"); return sb.ToString(); }

This isn't terrible. If the input is a null, we return that null, otherwise we convert some extended characters into their equivalents. I'm not sure "escape" is exactly the right name for this, but there's nothing absolutely wrong here. So let's instead turn our attention to UnifyString.

First, I'd like you to take a moment and ask yourself: what should a method called UnifyString do? What's the purpose of a method with that name?

Compare what you expect with this implementation:

public static string UnifyString(string aInputString, int aMaxLength, bool aEscape = true) { string result = ""; if(aEscape) result = Escape(aInputString); if (aInputString != null && aInputString.Length > aMaxLength) result = aInputString.Substring(0, aMaxLength); return result; }

Before we even get into the code, we have to comment on the use of Hungarian notation, which helps us distinguish which variables are arguments versus variables. I don't like it, but it's at least better than encoding types in the prefix, but we've looked at two methods thus far and already we don't agree as to whether or not this is an important standard.

But UnifyString is a substring tool, which should have been immediately clear from name alone, and why would you expect anything different? Were you expecting a join method or something? It's a substring that sometimes escapes characters! And, also, most important, it doesn't actually work as intended.

If aEscape is true (which is both its default value, and also the only way in which this method is ever called), all the extended characters are replaced with equivalents and stored in result. Then, if aInputString is not null and longer than a maximum length, we substring it and store that in result- right over top the escaped characters.

So the way this method works: if the input string is shorter than maxlength, we return that string with the extended characters replaced (possibly increasing its length beyond max length in the process), or we return an unescaped version of the string no longer than maxlength.

Erik writes:

The codebase is full of stuff like this, especially ill-named and illogical stuff. I already came close to sending in code several times, but this high-density bug-fest (which is solely running on pure luck and ignorance) pushed me over the edge.

[Advertisement] Utilize BuildMaster to release your software with confidence, at the pace your business demands. Download today!