Programmer Musings

Misleading error checking examples

I'm finally getting back to my blog after YAPC::NA. It's been a while since I wrote, so I figured I would start off easy.

Many months ago, I stumbled across yet another article describing how hard exceptions are (The Old New Thing : Cleaner, more elegant, and harder to recognize). It's an old article and I'm pretty sure I had read it before, but this time it struck a different cord for me.

The author carefully does not make the argument that exceptions are bad, he just states the getting exceptions right is harder that getting error-return checking right. His main argument boils down to the following two pieces of code:

NotifyIcon CreateNotifyIcon()

{

    NotifyIcon icon = new NotifyIcon();

    icon.Text = "Blah blah blah";

    icon.Visible = true;

    icon.Icon = new Icon(GetType(), "cool.ico");

    return icon;

}

NotifyIcon CreateNotifyIcon()

{

    NotifyIcon icon = new NotifyIcon();

    icon.Text = "Blah blah blah";

    icon.Icon = new Icon(GetType(), "cool.ico");

    icon.Visible = true;

    return icon;

}

The author states that the first is wrong and the second is right and this is subtle and hard to notice. He also uses a completely different example for showing good and bad error-checking code, where the mistakes are much more blatant. Several of the follow up comments take the author to task for flaws in his arguments and for specific details of the examples. My problem is that he did not do an accurate comparison. He compared apples to oranges.

Let's re-write these examples in the check error return style. I am going to modify this to be C++-specific, so that I can get the new operator not to throw an exception. I'll use RAII to simplify the memory management. Let's also assume that we want to return a null pointer on error.

Now, let's start with the bad code example:

NotifyIcon CreateNotifyIcon()

{

    std::auto_ptr icon( new(std::nothrow) NotifyIcon() );

    if(!icon)

    {

        return 0;

    }

    // Needed to add some error checking here for memory allocation

    if(!icon.SetText( "Blah blah blah" )

    {

        return 0;

    }

    icon.Visible = true;

    icon.Icon = new (std::nothrow) Icon(GetType(), "cool.ico");

    if(!icon.Icon)

    {

        return 0;

    }


    return icon.release();

}

The correct version of this code looks like this:

NotifyIcon CreateNotifyIcon()

{

    std::auto_ptr icon( new(std::nothrow) NotifyIcon() );

    if(!icon)

    {

        return 0;

    }

    // Needed to add some error checking here for memory allocation

    if(!icon.SetText( "Blah blah blah" )

    {

        return 0;

    }

    icon.Icon = new (std::nothrow) Icon(GetType(), "cool.ico");

    if(!icon.Icon)

    {

        return 0;

    }

    icon.Visible = true;


    return icon.release();

}

Once again the difference between the correct code and the bad code is one line moved a little. It may even be more subtle here due to the increase in number of lines of code.

Lest the Java people get the idea that this would be easier in Java, I'll recast the broken code in Java by handling the out of memory exception immediately.

NotifyIcon CreateNotifyIcon()

{

    NotifyIcon icon = null;

    try

    {

        icon = new NotifyIcon();

    }

    catch( java.lang.OutOfMemoryError e)

    {

        return null;

    }


    // Needed to add some error checking here for memory allocation

    if(!icon.SetText( "Blah blah blah" )

    {

        return null;

    }

    icon.Visible = true;

    try

    {

        icon.Icon = new Icon(GetType(), "cool.ico");

    }

    catch( java.lang.OutOfMemoryError e)

    {

        return null;

    }

    return icon;

}

To my eyes, the subtle logic error of setting the visible flag too soon is independent of the form of error recovery used. That example is a red herring that adds nothing to the debate.