Common Tasks
Search
  • Advanced search options...
  • Search
News

Holy cow, I wrote a book!

Basics
Blogroll
Categories

Cleaner, more elegant, and wrong

MSDN Blogs > The Old New Thing > Cleaner, more elegant, and wrong

Cleaner, more elegant, and wrong

Rate This
22 Apr 2004 7:03 AM
  • Comments 84

Just because you can't see the error path doesn't mean it doesn't exist.

Here's a snippet from a book on C# programming, taken from the chapter on how great exceptions are. 

try {
  AccessDatabase accessDb = new AccessDatabase();
  accessDb.GenerateDatabase();
} catch (Exception e) {
  // Inspect caught exception
}

public void GenerateDatabase()
{
  CreatePhysicalDatabase();
  CreateTables();
  CreateIndexes();
}
Notice how much cleaner and more elegant [this] solution is.

Cleaner, more elegant, and wrong.

Suppose an exception is thrown during CreateIndexes(). The GenerateDatabase() function doesn't catch it, so the error is thrown back out to the caller, where it is caught.

But when the exception left GenerateDatabase(), important information was lost: The state of the database creation. The code that catches the exception doesn't know which step in database creation failed. Does it need to delete the indexes? Does it need to delete the tables? Does it need to delete the physical database? It doesn't know.

So if there is a problem creating CreateIndexes(), you leak a physical database file and a table forever. (Since these are presumably files on disk, they hang around indefinitely.)

Writing correct code in the exception-throwing model is in a sense harder than in an error-code model, since anything can fail, and you have to be ready for it. In an error-code model, it's obvious when you have to check for errors: When you get an error code. In an exception model, you just have to know that errors can occur anywhere.

In other words, in an error-code model, it is obvious when somebody failed to handle an error: They didn't check the error code. But in an exception-throwing model, it is not obvious from looking at the code whether somebody handled the error, since the error is not explicit.

Consider the following: 

Guy AddNewGuy(string name)
{
 Guy guy = new Guy(name);
 AddToLeague(guy);
 guy.Team = ChooseRandomTeam();
 return guy;
}

This function creates a new Guy, adds him to the league, and assigns him to a team randomly. How can this be simpler?

Remember: Every line is a possible error.

What if an exception is thrown by "new Guy(name)"?

Well, fortunately, we haven't yet started doing anything, so no harm done.

What if an exception is thrown by "AddToLeague(guy)"?

The "guy" we created will be abandoned, but the GC will clean that up.

What if an exception is thrown by "guy.Team = ChooseRandomTeam()"?

Uh-oh, now we're in trouble. We already added the guy to the league. If somebody catches this exception, they're going to find a guy in the league who doesn't belong to any team. If there's some code that walks through all the members of the league and uses the guy.Team member, they're going to take a NullReferenceException since guy.Team isn't initialized yet.

When you're writing code, do you think about what the consequences of an exception would be if it were raised by each line of code? You have to do this if you intend to write correct code.

Okay, so how to fix this? Reorder the operations. 

Guy AddNewGuy(string name)
{
 Guy guy = new Guy(name);
 guy.Team = ChooseRandomTeam();
 AddToLeague(guy);
 return guy;
}

This seemingly insignificant change has a big effect on error recovery. By delaying the commitment of the data (adding the guy to the league), any exceptions taken during the construction of the guy do not have any lasting effect. All that happens is that a partly-constructed guy gets abandoned and eventually gets cleaned up by GC.

General design principle: Don't commit data until they are ready.

Of course, this example was rather simple since the steps in setting up the guy had no side-effects. If something went wrong during set-up, we could just abandon the guy and let the GC handle the cleanup.

In the real world, things are a lot messier. Consider the following: 

Guy AddNewGuy(string name)
{
 Guy guy = new Guy(name);
 guy.Team = ChooseRandomTeam();
 guy.Team.Add(guy);
 AddToLeague(guy);
 return guy;
}

This does the same thing as our corrected function, except that somebody decided that it would be more efficient if each team kept a list of members, so you have to add yourself to the team you intend to join. What consequences does this have on the function's correctness?

Comments
  • Dennis
    22 Apr 2004 7:34 AM
    Another general design principle: use Finally, and never leave a function without the data being in a consistent state.
  • Jeff Clark
    22 Apr 2004 7:49 AM
    In the last example the problem is if an exception occurs during AddToLeague(guy) you have a guy on a team but he doesn't belong to the league. As in the previous post, use a finally to remove guy from the team before leaving.
  • CLP
    22 Apr 2004 8:03 AM
    Program for *transactions*... and not necessarily only database transactions, but program with the whole transaction concept in mind. Either an entire operation must complete successfully or the whole thing fails. Always ask what happens if only a part of the transaction fails. Program not only to correctly handle internal exceptions, but consider how your component might fit into a larger transaction. Is it possible to rollback what your code has just done if later code requests that it be cancelled? Think transactions...
  • Matt
    22 Apr 2004 8:32 AM
    I'm not familiar with C#, but I know that with C++, have a properly written destructor should take care of lots of these problems. In the examples above, for instance, the Guy object's destructor should remove him from any leagues and/or teams to which he belongs. Well-written destructors go a long way toward the production of exception-safe code.
  • Raymond Chen
    22 Apr 2004 8:42 AM
    All you did was push the problem somewhere else. In C++ the problem is knowing when to call "delete" (and therefore cause the destructor to run).
  • Christian Romney
    22 Apr 2004 8:52 AM
    Sorry:

    public class Guy
    {
    public LeagueTeam Team
    {
    set
    {
    try
    {
    value.Add(this);
    _team = value;
    }
    catch (Exception ex)
    {
    // rollback any possible effect
    if (null != value && value.Contains(this))
    {
    value.Remove(this);
    _team = null;
    }
    }
    }
    get
    {
    return _team;
    }
    }
    }
  • Raymond Chen
    22 Apr 2004 9:05 AM
    You forgot to rethrow the exception. And this only fixes the setting of the team. Adding to the league is still an exception point.
  • Dejan Jelovic
    22 Apr 2004 9:06 AM
    Do a Google search for Dave Abraham's exception safety guarantees.

    Dave is clearest thinkers on exception safety by far.

    Sometimes you can emply the "strong guarantee" which says that everything is transactional and that all functions should undo what they did if an exception is thrown.

    Sometimes that's not possible ("how do you unfire a booster rocket?") or not practical (performance) to have the strong guarantee, and then you have to go for the basic guarantee.

    And why do I have to type these longish comments inside this little edit box?

    Dejan
  • Christian Romney
    22 Apr 2004 9:11 AM
    Misread -- league. Gotcha.
  • Eric Lippert
    22 Apr 2004 9:11 AM
    This "worst practice" of designing bad error handling has security impacts as well. Imagine if instead of CreateDatabase(); CreateTable(); it was CreateUser(); CreateRandomPassword(); ! You don't want to get into situations where you create users with blank passwords and then die horribly.

    Or, worse, DebitCheckingAccount(); CreditSavingsAccount();

    Any time sensitive data that must be consistent is manipulated, you've got to get the error handling right.
  • Christian Romney
    22 Apr 2004 9:13 AM
    Quite right. Fogot to rethrow, but if you wrap the caught exception, how is the addition to the team a problem? In other words,

    public class Guy
    {
    public LeagueTeam Team
    {
    set
    {
    try
    {
    value.Add(this);
    _team = value;
    }
    catch (Exception ex)
    {
    // rollback any possible effect
    if (null != value && value.Contains(this))
    {
    value.Remove(this);
    _team = null;
    }
    throw new ApplicationException("Could not join guy to league.", ex);
    }
    }
    get
    {
    return _team;
    }
    }
    }
    The application is in a consistent state because the guy's team isn't set and the team doesn't have the guy. Am I missing something?
  • Raymond Chen
    22 Apr 2004 9:15 AM
    But if an exception is thrown adding the guy to the league, you now have a team with a guy that doesn't belong to the league.
  • Christian Romney
    22 Apr 2004 9:20 AM
    Team should handle adding the guy to the league as well inside the Add(Guy guy) so that when you join one, you join the other.
  • Raymond Chen
    22 Apr 2004 9:48 AM
    That works if there is only one thing to be done outside adding to the league (picking a team). But what if there are several things you need to do, like say, assign the guy a membership number and adding to the orientation meeting.
  • Christian Romney
    22 Apr 2004 9:57 AM
    Not sure where you're going with this, we could add requirements ad nauseum. :) How would you handle this?
Page 1 of 6 (84 items) 12345»