10

For a typical if...else wrapped with exception handling, is something like the following example a recommended practice to avoid code duplication?

try
{
    if (GetDataFromServer())
    {
        return ProcessData();
    }
    else
    {
        throw new Exception();
    }
catch(Exception ex)
{
    return null;
}

instead of...

try
{
    if (GetDataFromServer())
    {
        return ProcessData();
    }
    else
    {
        return null;
    }
}
catch(Exception ex)
{
    return null;
}

I know there is a slight performance hit, but I'm wondering if this is considered to be an acceptable practice. I currently do it the second method - especially in cases where I need to handle specific exceptions differently - but I was wondering if the first method is appropriate for simple cases.

migrated from codereview.stackexchange.com Apr 22 '14 at 16:26

This question came from our site for peer programmer code reviews.

  • if the method is small enough I'll just remove the else and return null outside of the trycatch block so I have to return null only once. – Fabio Marcolini Sep 14 '14 at 7:03
12

Using exception handling for flow control is discouraged by Microsoft.

And a round table on the topic is available.

That being said, C# supports doing so, and I suppose it depends on the condition encountered whether an exception is the most appropriate response.

  • 1
    It strikes me that this sort of thing is just trying real hard to not use events. – radarbob Apr 22 '14 at 2:39
  • @radarbob: How are events related to this? – grovesNL Apr 22 '14 at 6:44
  • @grovesNL - Throwing an exception at a specific point to call a certain method in a catch block? Quacks like an event to me. – radarbob Apr 22 '14 at 15:08
  • @radarbob: It's not an event. There are lots of sample use cases where this would be used, as discussed in the answer's roundtable link. – grovesNL Apr 22 '14 at 15:10
  • 1
    @radarbob Just a bit of clarification, exceptions were designed as a way of signaling the caller that something has occurred which the called method is unable to handle. However, an event must be listened for. An exception is a forced interruption of the normal flow of a program. An uncaught exception will cause the entire application to abend. – B2K Apr 22 '14 at 15:54
6

The performance hit is most probably negligible, as explained in this answer.

So let's go with the idea that performance isn't an issue. You're throwing System.Exception, just to move execution into the catch clause. Throwing a BadControlFlowThatShouldBeRewrittenException would probably be overkill though.

Let's break this down. We have:

  • Method GetDataFromServer (method names should be PascalCase in C#), which can possibly throw an exception, or return a bool.
  • If result was true, run ProcessData.
  • Return null otherwise.

Looks like the method where this code is written, is simply doing too many things. GetDataFromServer returning a bool looks like a design flaw, I'd be expecting that method to return the data it's getting from the server, some IEnumerable<SomeType> that would contain 0 or more items - i.e. happy path returns n items where n > 0, not-so-happy path returns 0 items, and unhappy path blows up with an unhandled exception, whatever that is.

That changes what the method looks like, quite a lot - again it's hard to tell whether this makes sense, because the original post only has one exit point (and thus wouldn't compile, as not all code paths return a value), so this is only a wild guess:

try
{
    var result = GetDataFromServer();
    return ProcessData(result);
}
catch
{
    return null;
}

Here you'd look at ProcessData and see that it's iterating the result, and returns null if there's no item in the IEnumerable.

Now why is the method returning null? Server was down? Is there a bug in the query? The connection string is using the wrong credentials? Whenever GetDataFromServer blows up with an exception you're not expecting, you're swallowing it, shoving it under the carpet and returning a null value. I'd recommend catching specific exceptions in this case, and log everything else; debugging will be much easier that way.

With a general catch clause that doesn't capture the exception, it gets pretty hard to diagnose anything. I'd minimally do this instead:

catch(Exception e)
{
    return null;
}

Now you can at least break and inspect e if things go wrong.


TL;DR: No, throwing and catching exceptions for flow control isn't a good idea.

  • This answer demonstrates exactly why I tried to keep my code generic: I didn't want to list out every single exception that I actually make in my code; I didn't want to list the actual method names; I didn't want to list the method declaration; I didn't want syntax suggestions. I had a single question about whether throwing exceptions for flow control was okay, which was promptly answered by B2K. I would be happy to debate this on meta. – grovesNL Apr 22 '14 at 14:57
  • 3
    sounds like this should have been a question for Programmers then. we review code, not ideas. – Malachi Apr 22 '14 at 15:45
2

in your first answer there is a performance hit that doesn't need to be there.

try
{
    if (GetDataFromServer())
    {
        return ProcessData();
    }
    else
    {
        throw new Exception();
    }
catch(Exception ex)
{
    return null;
}

when you exit the if statement to enter into the Catch statement when you don't have to makes the code switch directions, so to say.

if you want to return null; do it in the else statement not in a catch that is caught after being thrown from the else statement.

Probably doesn't apply to your Real code, but for the Generic code that you gave it does apply.

Standards say you shouldn't do this.

Standards say you should do it like this, (again based on Generic Code Given in OP)

if (GetDataFromServer())
{
    return ProcessData();
}
else
{
    Return null
}

and since you don't have any specific exceptions that you are catching you shouldn't even have a try catch here.

you want to see Exceptions when they occur so that you can fix the issue that creates the exception.

1

Why not the far simpler:

if (!GetDataFromServer()) return null;
ProcessData();

If an exception handler is going to exist it should be in ProcessData()

  • Why wouldn't I want to pass exceptions through ProcessData() to the uppermost level? – grovesNL Apr 22 '14 at 17:20
  • @grovesNL Nothing useful was being done with the exception here. – Loren Pechtel Apr 22 '14 at 17:23
  • 1
    How so? If ProcessData() throws an exception now it's not handled. I want it to return null at this level if ProcessData() throws an exception, without modifying ProcessData() itself. – grovesNL Apr 22 '14 at 17:30

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.