Defensive Programming Example #1

The other day I stumbled across this bit of C# code, intended to randomly return one of four messages.

// Choose one of 4 messages to display randomly
string theMsg = "";
Random r = new Random();
switch(r.Next(4))
{
    case 0: theMsg = "Hey!"; break;
    case 1: theMsg = "Whats up?"; break;
    case 2: theMsg = "Salutations."; break;
    case 3: theMsg = "Hail-"; break;
    default: throw new ArgumentException(); break;
}
return theMsg;

Few programmers would have trouble grokking what this code does. It’s intelligible and, in its current form, not too much of a maintenance headache. But defensive programming isn’t just about making your code intelligible; it’s about making your code tolerant to quick changes made under pressure, and incremental changes made over time, by people other than yourself, including the future version of yourself who no longer remembers the code.

In that light, this code has a lot of problems:

  • There’s a temporary variable (theMsg) which isn’t needed and which doesn’t add clarity.
  • The allocation of a Random object could fail.
  • The switch statement is hard-coded for a specific number of messages (4).
  • Each new message requires a separate case.
  • The messages are stored as hard-coded string literals.
  • The messages are sorted into case statements rather than stored as a collection.
  • Becomes a maintenance headache as the number of messages increases.

Let’s fast-forward a couple years and see how the code has evolved:

// Choose one of 4 messages to display randomly
string theMsg = "";
Random r = new Random();
// Update this when you add new messages!
int globalMessageCount = 6;

if (user_lang = "en-us")
{
    switch(r.Next(globalMessageCount))
    {
        case 0: theMsg = "Hey!"; break;
        case 1: theMsg = "Whats up?"; break;
        case 2: theMsg = "Salutations."; break;
        case 3: theMsg = "Hail-"; break;
        // Added new message for R4320 update
        case 4: theMsg = "Hola!"; break;
        case 5: theMsg = "Greetings."; break;
        default: throw new ArgumentException(); break;
    }
}
else if (user_lang = "es")
{
    switch(r.Next(globalMessageCount))
    {
        case 0: theMsg = "Hola!"; break;
        case 1: theMsg = "Que paso?"; break;
        case 2: theMsg = "Como esta usted?"; break;
        case 3: theMsg = "Ay gringo!"; break;
        // Added new message for R4320 update
        //case 4: theMsg = "Soy capitain?"; break;
        //case 5: theMsg = "Tengo hombre."; break;
        default: throw new ArgumentException(); break;
    }
}
return theMsg;

Yikes! The big thing that happened is they added some sort of half-assed globalization support for Spanish in addition to English. They’ve also gone in an added a couple new messages as part of a feature update. Some well-meaning soul attempted to do something with the hard-coded “13″ by moving it into a named variable, but of course that just makes it worse. We can infer from the “Update this when you add a new message comment” that somebody got burnt by this already.

Like a splinter that festers and gets infected, the innocuous piece of code we started with has now evolved into a piece of technical debt. If the business case calls for not just a handful but dozens or hundreds or thousands of messages, this code is completely unworkable: a true coding horror. (Animated, even.)

All of this foolishness could’ve been avoided by applying a bit of defensive programming spackle to the code in its Day One incarnation.

Fixing this becomes a simple matter of removing the cruft and boiling the fragment down to its bare essentials. Anytime a switch statement is used to retrieve a piece of data at a particular index, consider using an array or other collection class and indexing directly, as follows:

// Choose one of 4 messages to display randomly
string [] messages = new { "Hey!", "Whats up?", "Salutations.", "Hail-" }
Random r = new Random();
return messages[r.Next(messages.Length)];

Now we can add/remove messages transparently, without having to tweak obscure count variables or add new case statements. Our strings are still hard-coded, and we don’t like that, but since we’ve got our messages into some sort of collection, we can source easily source them from an external database or file in the future:

// Choose one of 4 messages to display randomly
string [] messages = LoadMessagesFromFile();
Random r = new Random();
return messages[r.Next(messages.Length)];

That in turn will make adding globalization support a lot easier:

// Choose one of 4 messages to display randomly
string [] messages = LoadLocalizedMessagesFromFile(user_lang);
Random r = new Random();
return messages[r.Next(messages.Length)];

This is better code in the sense that it’s both easy to understand and difficult for future maintainers to screw up. And that, ladies and germs, is what defensive programming is all about.

Posted by on July 2, 2012 in Coding, ,

Comments

  • Fabio 'Petrucio' Stange says:

    If I was faced with such a code, I’d refactor to something like this instead:

    // Choose a random message to display
    string [] messages = new {
    tr(“Hey!”),
    tr(“Whats up?”),
    tr(“Salutations.”),
    tr(“Hail-”),
    }
    return messages[(new Random()).Next(messages.Length)];

    I think it’s MUCH better to have strings in the code than to fetch them from somewhere else, because the programmer can’t readily see what the strings are when reading the code.

    It’s much easier and faster to read, does not requires the programmer to go elsewhere to check the strings, and it gives you the same power to seamlessly translate the strings. (The tr() macro/function DOES read stuff from external files transparently)

    I’m willing to be proven, but I prefer my code to yours.

    • Coding the Wheel says:

      Thanks for the insightful comment. The only problem I see with your solution (other than the lack of a widely-available “tr” macro) is that the messages are still tightly coupled with the code. Sure, you’ve got one-off localization via tr(), but adding or removing a string means recompiling the code. And how does this solution scale as the number of messages grows from 4, to a dozen, to a couple hundred? Just dump them all in the code? What if non-programmers want to add, remove, or change messages?

      This is basically the reason why Visual Studio, Xcode, etc. support the concept of string resources, and why in production code you’ll typically see string identifiers (IDS\_MYSTRING1) in place of string literals; they want all their “string stuff” in one place, whether so they can eventually ship it off to a translator, or to prevent customers from peeling open the executable with a hex editor to discover all those beautifully preserved hard-coded strings. Storing strings in a file or a database is another approach.

      However for simple cases (setting a piece of localized button text) I agree that tr() or string-by-identifier approaches are fine, and it helps to be able to see tr(“Submit!”) as opposed to SetDlgItemText(IDS\_STR\_SUBMIT) or what have you.

      • Petrucio says:

        I agree that the best approach would depend on what you want to do. If you want non-programmers adding strings to the list, then I concede that it would be best to have that read from outside. But I’ll be hard-pressed to find a real-world situation that calls for that everytime you want a translation – better write a special case for that then.

        We use the tr() approach here, and it can be shipped off to translators just fine. A script parses all the code after doing the daily build looking for the tr()s, and the translators have a nice translation file waiting for them to update the next morning. Non-programmers do these translations for us, and it’s in the thousands, not hundreds. And mostly of the simple case type you mentioned, and it works fine. They can even be translated from english(programmer) to english(marketting/user), to make sure those crazy programmers do not directly address your users, although we don’t go there. And I really do prefer to read the strings as I program then to read IDS_STR_SUBMIT, as you said. I’ve tried some globalization solutions, and this is the only one so far that hasn’t annoyed me to no end.

        If you really care about not letting your users looking at your strings, then fine, I concede it’s better to use resources.

        • Anonymous says:

          Where I work we actually use both tr and string resource IDS_BLAH here. And hard-coded strings. Interesting discussion guys!

      • Petrucio says:

        The tr() solution I use comes from Qt, by the way, for the curious. I don’t think Qt has decent bindings for C#.
        Not that it matters, the approach could be implemented in-house for any language, and that was my point.

  • Fabio 'Petrucio' Stange says:

    Not to mention that `LoadLocalizedMessagesFromFile` is another function, specific to this piece of code and not generic, so your refactor creates more code to maintain that is at first apparent. A `translate(string)` type function is generic, so the refactor would not add more code to maintain.

  • Chis S. says:

    This is why I like dependency injection, because it handles this so cleanly.

  • Defensive programming is something that I want to achieve for my programming language project, as well as for my personal website.

  • Helen Neely says:

    Thanks for this post. But my little concern is that reading String values from external source may be an over kill for just a few strings that can easily be placed in an array. But if however, you have a very long list of values that will be changed often or not known from the outset, then I think reading from a file or database would be a perfect idea.

    I will definitely consider your tips next time.
    Nice job.

  • Anonymous says:

    yugyugyugyugygyug

    90ii

    i9i

  • Leave a Reply