S h o r t S t o r i e s

// Tales from software development

Do not return false to indicate a method nearly worked

leave a comment »

I’ve been tasked with resolving some bugs in the Order Management Processing System (OMPS) of an online retailer. The code was written three years ago in .NET 1.1 by a bunch of guys who were used to developing in Visual Basic 6.

One bug took about 2 days to diagnose because the manifestation of the bug was a long way downstream of the cause. The symptom was a string operation on a property that failed because the string was null. The actual cause was that the object that the property belonged to had been instantiated but not populated with data.

The path through the code that resulted in the exception was something like this:

{
    // Send a notification email
    if (user == null)
    {
        user = GetUser();
    }
    SendEmail(user.Email);
}

private User GetUser()
{
    return new Logon(m_Accountid);
}

public class Logon
{
    public Logon(int accountId)
    {
        LoadData(accountId);
    }

    private bool LoadData(int accoundId)
    {
        if (DataAccess.GetUserDetails(accountId)
        {
            // populate this object
            return true;
        }
        else
        {
            return false;
        }
    }
}

What’s notable about this code is the way that it attempts to handle a failure to locate the requested details: the LoadData() method returns false. But there is very little that can be usefully done at this stage other than throwing an exception. Even worse than attempting to handle this situation by simply passing the problem back to the caller is that the calling code doesn’t bother checking the return value anyway but simply exits the Logon object constructor.

The result is that the code at the top of the call stack is given an instantiated but unpopulated object. This is worse than useless – it’s dangerous as the code will continue to execute with an invalid object. Sooner or later the code will probably fail. In this scenario the code in the SendEmail method fails because it cannot handle an email address of null.

I discussed this code with a colleague saying that I thought it was probably the worst .NET code I’d ever seen. He argued that he’s seen worse coding but then I pointed out that this wasn’t simply bad coding – this was a complete failure to use the .NET programming paradigm. The failure to access the user details in the LoadData() method is a perfect example of when and where an exception should be thrown but the programmer responsible instead used a return code to indicate failure but then didn’t actually bother checking the return code. This half-hearted approach results in an object that exists when it shouldn’t. Clearly the LoadData() method should either complete successfully or an exception should be thrown, returning false is neither useful or safe.

Advertisements

Written by Sea Monkey

October 26, 2008 at 8:00 pm

Posted in Debugging, Development

Tagged with

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

%d bloggers like this: