Catching exceptions correctly .net

Question

I'm working in a new place and seeing a lot of things in the code I'm not comfortably with.

I've been seeing a lot of code like this:

try
{
    dAmount = double.Parse(((TextBox)control.Items[someRow].FindControl("txtControl")).Text);
}
catch
{
    dAmount = 0;
}

I read stackoverflow and the like frequently and I'm aware that is not right, but I'm debating myself about the best way to handle this.

The developer obviously thought setting dAmount=0; was a good way to handle the exception, but seeing how double.Parse can only throw 3 exceptions(ArgumentNullException, FormatException, OverflowException) and if we add the NullReferenceException in case the FindControl or other object comes back null, then it seems to me I get to cover all the cracks, however the code looks kind of ugly, and I'm looking for suggestions, a better approach maybe?

This is what I came out with

try
{
    dAmount = double.Parse(((TextBox)control.Items[someRow].FindControl("txtControl")).Text);
}
catch ( NullReferenceException nullRefEx )
{
    dAmount = 0;
    nwd.LogError("***Message: " + nullRefEx.Message + " ***Source: " + nullRefEx.Source + " ***StackTrace: " + nullRefEx.StackTrace);
}
catch ( ArgumentNullException argNullEx )
{
    dAmount = 0;
    nwd.LogError("***Message: " + argNullEx.Message + " ***Source: " + argNullEx.Source + " ***StackTrace: " + argNullEx.StackTrace);
}
catch ( FormatException frmEx )
{
    dAmount = 0;
    nwd.LogError("***Message: " + frmEx.Message + " ***Source: " + frmEx.Source + " ***StackTrace: " + frmEx.StackTrace);
}
catch ( OverflowException ovrEx)
{
    dAmount = 0;
    nwd.LogError("***Message: " + ovrEx.Message + " ***Source: " + ovrEx.Source + " ***StackTrace: " + ovrEx.StackTrace);
}

BTW I don't control the logging function is from another team in here, I know is kind of ugly.

What about saving the exception in a general Exception and having only one nwd.LogError call at the end? any suggestions are welcome.

Answer 1

Catch a single exception that covers all those instances. The message in the log file should distinguish which type of exception was thrown.

string s = ....;
double d = 0;
try {
    d = Double.Parse(s);
} catch (Exception ex) {
    //Set default value
    nwd.LogError("***Message: " + ex.Message + " ***Source: " + ex.Source + "***StackTrace: " + ex.StackTrace);
}

The key the other answers are missing is that he wants to log the exception... TryParse won't tell you why it didn't parse correctly, it just returns false and 0 in the out param.

The real question here is... is zero actually special? I.e. is 0 not a valid number for a person to put into the text box? How would the rest of the program tell whether the user entered a zero, or the zero was a result of the default value being returned on parse error?

The better plan, and one I use when I am getting a value object and might fail is to use a nullable...

string s = ....;
double? d = null;
try {
    d = Double.Parse(s);
} catch (Exception e) {
    nwd.LogError("***Message: " + ex.Message + " ***Source: " + ex.Source + "***StackTrace: " + ex.StackTrace);
}
return d;

Now the caller knows for a fact that an error occurred when you return null (versus some "magic value"). Using "magic values" unnecessarily complicates the semantics.

Even better is to simply let the error propagate (or re-throw it) to where it can actually be handled reasonably. What part of your code currently checks for that 0 "magic value" and does something different based on it? That's where your catch should be (or, catch and log and then rethrow).

Answer 2

Try/Catch isn't there to control flow of your methods. If you can't redesign whole thing then I'd do something like this.

public double Parse(...)
{
    double value = 0;
    string text = ((TextBox)control.Items[someRow].FindControl("txtControl")).Text;
    double.TryParse(text, out value);
    return value;
}

Answer 3

You should use Double.TryParse instead.

double number;
if (Double.TryParse(value, out number))
   dAmount = number;
else
   dAmount=0;

This is cleaner and avoids exceptions altogether.

Answer 4

Don't use finally because that will always set dAmount = 0, even if an exception isn't thrown. If you want to catch a general Exception, then a single catch (Exception ex) will suffice. If you want to catch those specifically, I would do the following:

try
{
    // code here
    ...
}
catch (Exception ex)
{
    if (ex is NullReferenceException || ex is ArgumentNullException 
            || ex is FormatException || ex is OverflowException)
    {
        // handle ex
        ...
        return;
    }
    throw;
}

In your case, your best option is to probably use Double.TryParse, like so:

double amount;
if(Double.TryParse(someValue, out amount)
{
    // do something with amount...
}
else
{
    // handle the error condition here
}

Answer 5

1. You're doing exactly the same thing with all of your exceptions, which rather defeats the purpose of catching them all separately. You should be catching different exceptions if you intend to actually do something different with each type.

2. Your finally block will run and zero out your value even if there are no exceptions.

3. You shouldn't be using exceptions for control flow in the first place; you should be writing the code so that no exceptions are thrown at all if the user inputs invalid data, rather than throwing and catching an exception for a non-exceptional use-case. In this context that's as simple as using double.TryParse.

4. Exceptions such as null reference/null argument exceptions, overflow exceptions, etc. should pretty much never be caught at all outside of a top level method that logs any fatal exceptions before failing gracefully. These are boneheaded exceptions; if you're getting them it's a sign that you have a bug in your code that you need to fix, not a problem that you should try to sweep under the rug and continue executing your program.