Is It Always Bad To Swallow Exceptions?

0 comments suggest edit

Reading this post from Jayson’s blog caught my attention for two reasons. First, his very strong reaction to some code that swallows an exception. Second, the fact that I’ve written such code before.

Here is the code in question.

public bool IsNumeric(string s)

{

    try

    {

        Int32.Parse(s);

    }

    catch

    {

        return false;

    }

    return true;

}

Jayson’s proposed solution is…

I personally would use double.TryParse() (and downcast accordingly depending on the result) at the very least. At the very most I’d break the string down to a char array, and walk the array calling one of the (very) useful static char.Is<whatever> methods…first non<whatever> value, break out of the loop and return false. I’ve posted before about the speed at which the framework can process char data…it’s very fast and effecient (sic).

For the sake of this discussion, let’s assume the method was intended to be IsInteger(). Using Int.Parse() to test if a string is a number doesn’t make sense since it immediately chokes on 3.14 (get it? Chokes on Pi. Get it? Damn. No sense of humor). If indeed this method was intended to be IsNumeric then I would suggest using double.TryParse and the discussion is finished.

Now in general, I agree with Jayson and often raise fits when I see an exception blindly swallowed. However, when you only deal in absolutes, you start to become a robot (yes, I am resisting the offhand political joke here). For every absolute rule you find in programming (or anywhere for that matter), there is often an example case that is the exception to the rule. As they say, the exception proves the rule.

The problem with simply parsing the string character by character is that it’s quite easy to make a mistake. For example, if you simply called char.IsNumber() on each character, your code would choke on “-123”. That’s certainly an integer.

Also, what happens when you want to extend this to handle hex numbers and thousands separators. For example, this code snippet shows various ways to parse an integer.

Console.WriteLine(int.Parse(“07A”, NumberStyles.HexNumber));

Console.WriteLine(int.Parse(“-1234”, NumberStyles.AllowLeadingSign));

Console.WriteLine(int.Parse(“1,302,312”, NumberStyles.AllowThousands));

Console.WriteLine(int.Parse(“-1302312”));

This is one of those cases where the API failed us, and was corrected in the upcoming .NET 2.0. In .NET 2.0, this is a moot point. But for those of us using 1.1, I think this is a case where it can be argued that swallowing an exception is a valid workaround for a problem with the API. However, we should swallow the correct exception.

Since there is no int.TryParse() method, I’d still rather rely on the API to do number parsing than rolling my own. It’s not that I don’t think I am capable of it, but I have a much smaller base of testers than the framework team. Here’s how I might rewrite this method.

public bool IsInteger(string s, NumberStyles numberStyles)

{

    if(s == null)

        throw new ArgumentNullException(“s”, “Sorry, but I don’t do null.”);

 

    try

    {

        Int32.Parse(s, numberStyles);

        return true;

    }

    catch(System.FormatException)

    {

        //Intentionally Swallowing this.

    }

    return false;

}

So in 99.9% of the cases, I agree with Jayson that you should generally not swallow exceptions, but there are always the few cases where it might be appropriate. When in doubt, throw it. In the rewrite of this method, notice that I don’t catch ALL exceptions, only the expected one. I wouldn’t want to swallow a ThreadAbortException, OutOfMemoryException, etc…

I would also put a //TODO: in there so that as soon as the polish is put on .NET 2.0, I would rewrite this method immediately to use int.TryParse() and make everybody more comfortable.

This is a case where I do feel uneasy using an exception to control the flow, but that uneasiness is ameliorated in that it is encapsulated in a tight small method. Also, one objection to this post I can anticipate is that it is freakin’ easy to parse an integer, so why not roll your own? While true, the principle remains. What if we were discussing parsing something much more difficult? For exampe, suppose we were instead discussing a method IsGuid(). Now you have to deal with the fact there isn’t even a Guid.Parse() method. You have to pass the string to the constructor of the Guid which will throw an exception if the string is not in a valid format. Yikes! I thought constructors were never supposed to throw exceptions.

In this case, I’d probably prefer not to roll my own Guid parsing algorithm, instead relying on the one provided. Why write code that already exists?

So Jayson, in general you are right, but please don’t beat me to death with a wet noodle if you see something like this in my code. ;)

Found a typo or error? Suggest an edit! If accepted, your contribution is listed automatically here.

Comments

avatar

6 responses

  1. Avatar for Marty Thompson
    Marty Thompson August 10th, 2005

    Strangely enough, Jeff Atwood had a similar topic on his blog post yesterday(http://www.codinghorror.com/blog/archives/000358.html).



    The thing about using exceptions in the case you are describing is how expensive they are. You can mention the pitfalls of early optimization and all that comes along with it, but the fact of the matter is that numeric validation can easily be done using regular expressions(and it's cheaper).

  2. Avatar for jayson knight
    jayson knight August 10th, 2005

    *puts away the wet noodle...for now*



    That was indeed (what I tried to make) the main point of my post...exceptions are very expensive to generate, and in the case of a utility method like this that could potentially be called quite a bit, most definitely performance would suffer. I won't argue with swallowing exceptions occasionally (i.e. if it's a benign exception and won't bring an app to its knees), or very rarely using an exception to dicate control of flow...but most definitely not a combination of both, and very much most definitely not in a method like the first one where potentially thousands of exceptions could be generated per second.



    I really need to start digging into 2.0, I didn't realize that TryParse had been extended to more datatypes (which should have shipped in 1.x...but who knows what the collective's motivations where behind that).

  3. Avatar for Haacked
    Haacked August 10th, 2005

    Right, but it isn't a NEVER EVER issue.



    Suppose that method was used in a tight loop and called very often, but 95% or more of the strings passed to that method are valid numerics. In that case, I would probably leave the method as is.



    But if only 50% or less were numerics, then I would almost definitely look for another solution.



    In both cases, I would measure first, then make the optimization, then measure again.



    The regular expressions for matching integers are fairly easy, but get more complicated if you want to allow commas as the thousand separators. Of course, it's doable, but I hate to roll my own solution to something that is already superbly tested.



    For example, ever try validating floating point numbers using Regex? It's a bit more challenging than many people realize. Often, you take the naive approach ([-+]?[0-9]*\.?[0-9]*) which ends up being wrong.



    Also, as I pointed out, what if we were trying to validate something even more complicated? For example, a GUID. You could use a Regex



    ^(\{){0,1}[0-9a-fA-F]{8}\-[0-9a-fA-F]{4}\-[0-9a-fA-F]{4}\-[0-9a-fA-F]{4}\-[0-9a-fA-F]{12}(\}){0,1}$



    but how readable is that?



    But I am in total agreement that the code Jayson displayed was faulty in that it blindly caught ALL exceptions, rather than the one exception it was interested in

  4. Avatar for protected virtual void jaysonB
    protected virtual void jaysonB August 14th, 2005

    I posted earlier about a troublesome piece of code I came across that parses a string to see if it&rsquo;s...

  5. Avatar for Jon Galloway
    Jon Galloway August 14th, 2005

    I think the approach depends on the expected liklihood of the input value passing validation. If it's expected that most of the time the value will pass, I'd just handle the exception to avoid the unnecessary validation operations each time a we verify an integer is really an integer. If the input is unknown (library or component code) or likely to fail, I'd hit regexlib.com and at least use a regex that's likely had some testing.



    I just made a data change on an app that does millions of casts on nullable database columns, and converting to a simple null check with an exception check yielded the best performance.

  6. Avatar for Andrea
    Andrea March 5th, 2018

    In general: NEVER swallow exceptions, or the psycopath who knows where you live will come for you.