views:

171

answers:

9

Hi,

What would a piece of code which "uses exceptions to control flow" look like? I've tried to find a direct C# example, but cannot. Why is it bad?

Thanks

+4  A: 

Bad

The below code catches an exception that could easily be avoided altogether. This makes the code more difficult to follow and typically incurs a performance cost as well.

int input1 = GetInput1();
int input2 = GetInput2();

try
{
    int result = input1 / input2;
    Output("{0} / {1} = {2}", input1, input2, result);
}
catch (OverflowException)
{
    Output("There was an overflow exception. Make sure input2 is not zero.");
}

Better

This code checks for a condition that would throw an exception, and corrects the situation before the error occurs. This way there is no exception at all. The code is more readable, and the performance is very likely to be better.

int input1 = GetInput1();
int input2 = GetInput2();

while (input2 == 0)
{
    Output("input2 must not be zero. Enter a new value.");
    input2 = GetInput2();
}

int result = input1 / input2;
Output("{0} / {1} = {2}", input1, input2, result);
Dan Tao
So it boils down to not using exceptions to perform logic to cure the exception and doing validation before hand? Therefore, exceptions should be used for something truly exceptional and not really possible to forsee.
dotnetdev
@dotnetdev: Typically, yes. Although sometimes this can be masked in other ways...
Reed Copsey
@dotnetdev: yes, although you usually do know what kind of exceptions are likely. If you call a fn that does something with files on disk, file exceptions are a possibility. etc.
dthorpe
+9  A: 

By definition, an exception is an occurrence which happens outside the normal flow of your software. A quick example off the top of my head is using a FileNotFoundException to see if a file exists or not.

try
{
    File.Open(@"c:\some nonexistent file.not here");
}
catch(FileNotFoundException)
{
    // do whatever logic is needed to create the file.
    ...
}
// proceed with the rest of your program.

In this case, you haven't used the File.Exists() method which achieves the same result but without the overhead of the exception.

Aside from the bad usage, there is overhead associated with an exception, populating the properties, creating the stack trace, etc.

Eric
Ouch, not a good example. File.Exists() is *not* a reliable alternative on a multi-tasking operating system.
Hans Passant
True. Still, the contrast between relying on an exception vs. using a framework-provided method (pun intended) is a valid one.
Eric
@eric What pun?
Malfist
@Malfist, "method" meaning (in the general English sense of the word) a means of accomplishing something vs. (in an object-oriented context) an operation supported by a class. Not my best work, but I take 'em where I can get 'em. :-)
Eric
Hans: If you just checked that a file exists, and then you go to open it and get a `FileNotFoundException`, would it be better to try to create a new file in its place, or throw an exception?
StriplingWarrior
+3  A: 

Here's a common one:

public bool TryParseEnum<T>(string value, out T result)
{
    result = default(T);

    try
    {
        result = (T)Enum.Parse(typeof(T), value, true);
        return true;
    }
    catch
    {
        return false;
    }
}
Toby
That's a good example, especially when a good alternative - `TryParse` - already exists
ChrisF
+1 - I've seen this in code I've worked on - it got called loads of times during a sort and caused performance issues. Using TryParse like ChrisF says removed the performance issue. However, I believe this was the only way pre .NET 2.0.
Alex Humphrey
A: 

Here's a pretty trivial example that I think still shows off what using exceptions for flow control looks like:

public void DoSomething()
{
    int x;
    try
    {
        x = GetValueOfX();
    }
    catch
    {
        x = 5;
    }

    // do something meaningful to X
}
Anna Lear
+6  A: 

It's roughly equivalent to a goto, except worse in terms of the word Exception, and with more overhead. You're telling the code to jump to the catch block:

bool worked;
try
{
    foreach (Item someItem in SomeItems)
    {
        if (someItem.SomeTestFailed()) throw new TestFailedException();
    }
    worked = true;
}
catch(TestFailedException testFailedEx)
{
    worked = false;
}
if (worked) // ... logic continues

As you can see, it's running some (made-up) tests; if they fail, an exception is thrown, and worked will be set to false.

Much easier to just update the bool worked directly, of course!

Hope that helps!

Kieren Johnstone
+1 While some of the other examples are also controlling flow, I think this is the nightmare scenario that's being warned against.
Greg
+1  A: 

I'm not fond of C# but you can see some similarities between try-catch-finally statements and normal control flow statements if-then-else.

Think about that whenever you throw an exception you force your control to be passed to the catch clause. So if you have

if (doSomething() == BAD) 
{
  //recover or whatever
}

You can easily think of it in terms of try-catch:

try
{
  doSomething();
}
catch (Exception e)
{
  //recover or do whatever
}

The powerful thing about exception is that you don't have to be in the same body to alter the flow of the program, you can throw an exception whenever you want with the guarantee that control flow will suddently diverge and reach the catch clause. This is powerful but dangerous at the same time since you could have done actions that need some backup at the end, that's why the finally statement exists.

In addition you can model also a while statement without effectively using the condition of it:

while (!finished)
{
  //do whatever
}

can become

try
{
  while (true)
  {
     doSomethingThatEventuallyWillThrowAnException();
  }
}
catch (Exception e)
{
  //loop finished
}
Jack
Excellent remark regarding that the exception catch could be much father up the stack.
overslacked
+1  A: 

Probably the grossest violation I've ever seen:

// I haz an array...
public int ArrayCount(object[] array)
{
    int count = 0;
    try
    {
        while (true)
        {
            var temp = array[count];
            count++;
        }
    }
    catch (IndexOutOfRangeException)
    {
        return count;
    }
}
Tesserex
Oh man, have you seriously *seen* this?
Dan Tao
Actually I think I stole it from The Daily WTF. So I only "saw it" on a website.
Tesserex
+1  A: 

I'm currently working with a 3rd party program that does this. They have a "cursor" interface (basically an IEnumerable alternative), where the only way to tell the program you're finished is to raise an exception. The code basically looks like:

// Just showing the relevant section
bool finished = false;

public bool IsFinished()
{
    return finished;
}

// Using something like:
// int index = 0;
// int count = 42;

public void NextRecord()
{
    if (finished)
        return;

    if (index >= count)
        throw new APIProgramSpecificException("End of cursor", WEIRD_CONSTANT);
    else
        ++index;
}

// Other methods to retrieve the current value

Needless to say, I hate the API - but its a good example of exceptions for flow control (and an insane way of working).

Reed Copsey
+1  A: 

A module developed by a partner caused our application to take a very long time to load. On closer examination, the module was looking for a config file at app startup. This by itself was not too objectionable, but the way in which it was doing it was outrageously bad:

For every file in the app directory, it opened the file and tried to parse it as XML. If a file threw an exception (because it wasn't XML), it caught the exception, squelched it, and tried the next file!

When the partner tested this module, they only had 3 files in the app directory. The bonehead config file search didn't have a noticeable effect on the test app startup. When we added it to our application, there were 100's of files in the app directory, and the app froze for nearly a minute at startup.

To add salt to the wound, the name of the config file the module was searching for was predetermined and constant. There was no need for a file search of any kind.

Genius has its limits. Stupidity is unbounded.

dthorpe
So (typeof(Genius) == "Array") == (typeof(Stupidity) == "List") ?
Justin Niessner
Wasn't there a similar control flow fixed that had to do with looking for for translated resources?
Jeroen Pluimers
Yes, that may be the same case as this.
dthorpe