Is there any valid reason to ever ignore a caught exception

Wow, I just got back a huge project in C# from outsourced developers and while going through my code review my analysis tool revealed bunches of what it considered bad stuff. One of the more discouraging messages was:

Exceptions.DontSwallowErrorsCatchingNonspecificExceptionsRule  : 2106 defects 

The developers assure me they had good reason for all the empty catch blocks, that sometimes the try with empty catch blocks are just there to ignore useless exceptions and keep the application from crashing. I feel this is a cop out and complete BS. Some of the examples I actually looked up were database calls where the record was being saved to the database, and in this case, if an exception was ignored, the user would get back an okay prompt, think everything was okay, and continue on with their work. In reality, their work was never saved. I think this is absolutely the most horrible kind of error. In this case, they are completely wrong in throwing that code in a try with an empty catch block. But my question is, "Is this EVER acceptable in ANY situation?" I think not, but I've been known to be wrong.

While there are some reasonable reasons for ignoring exceptions; however, generally it is only specific exceptions that you are able to safely ignore. As noted by Konrad Rudolph, you might have to catch and swallow an error as part of a framework; and as noted by osp70, there might be an exception generated by a framework that you know you can ignore.

In both of these cases though, you will likely know the exception type and if you know the type then you should have code similar to the following:

try {
  // Do something that might generate an exception
} catch (System.InvalidCastException ex) {
  // This exception is safe to ignore due to...
} catch (System.Exception ex) {
  // Exception handling
}

In the case of your application, is sounds like something similar might apply in some cases; but the example you give of a database save returning an "OK" even when there is an exception is not a very good sign.

I don't catch exceptions unless I plan to do something about them. Ignoring them isn't doing something about them.

I sometimes use a WebControl that is not compulsory for a page to display. If it fails, I don't want to prevent the page from displaying. An example of a non-critical WebControl would be one that displays an advertisement.

However, I do log the error. I just don't rethrow it.

My feeling is that any empty Catch Block needs a comment.

Possibly it's valid to ignore certain errors, but you need to document your reasons.

Also, you wouldn't want to make it a generic "catch (Exception e) { }".

You should catch only the specific error type that's expected there and is known to be safely ignored.

Generally no, in fact no in 99% of all cases, BUT

There are exceptions. One one project I worked on we used a third party library to handle a TWAIN device. It was buggy and under some hardware combinations would throw a null pointer error. However we never found any circumstances when it didn't actually manage to scan the document before it did that - so catching the exception was entirely rational.

So I think if it's your code that's throwing the exception then you should always check it, but if you're stuck with third party code then in some circumstances you may be forced to eat the exception and move on.

The other case where you can be excused for catching and ignoring exceptions is when you're unit testing.

public void testSomething(){
    try{
        fooThatThrowsAnException(parameterThatCausesTheException);
        fail("The method didn't throw the exception that we expected it to");
    } catch(SomeException e){
        // do nothing, as we would expect this to happen, if the code works OK.
    }
}

Note that, even though the catch block does nothing, it explains why.

Having said this, more recent testing frameworks (Junit4 & TestNG) allow you to specify the exception that is expected - which leads to somthing like this...

@Test(expected = SomeException.class)
public void testSomething(){
    fooThatThrowsAnException(parameterThatCausesTheException);
    fail("The method didn't throw the exception that we expected it to");
}
  

I guess from what I gather the best answer is that it can be somewhat acceptable but should be limited. You should try to use another alternative, and if you can't find another alternative, you should know enough about how the code works that you can expect specific exception types and not just use the blanket catch all "Exception". Documentation of the reason why this exception is ignored should be included in the form of an understandable comment.

in critical code, probably not, because the state of the program must always be exactly defined. like your database call example.

in noncritical code, sure, we do it too (we just display caught exceptions in a message box and keep running). maybe a plugin or module is failing, but the main program isn't affected. maybe a lexical_cast failed and there's a text glitch rendered to the screen. No need to halt the process.

One example of where I'd consider this acceptable is in some non-critical module of a critical application (say, in the sound feedback module of the space shuttle navigation system), for exceptions that should never happen, and cannot be handled more cleanly.

In those cases, you would not want to let that exception propagate and cause the whole application to fail (Sorry guys, no more navigation system, our beeping module crashed, and there was really nothing we could do about it).

Edited to say that in any of these cases, you'd at least want to log the event somewhere.

I think that if you have an empty catch block you need to document why it is empty so that the next developer will know. For example on server.transfer a web exception is sometimes thrown. I catch that and comment that we can ignore it because of the transfer call.

There are certainly circumstances where it's OK to catch a specific exception and do nothing. Here's a trivial example:

    public FileStream OpenFile(string path)
    {
        FileStream f = null;
        try
        {
            f = new FileStream(path, FileMode.Open, FileAccess.ReadWrite);
        }
        catch (FileNotFoundException)
        {
        }
        return f;
    }

You could also write the method this way:

    public FileStream OpenFile(string path)
    {
        FileStream f = null;
        FileInfo fi = new FileInfo(path);
        if (fi.Exists)
        {
            f = new FileStream(path, FileMode.Open, FileAccess.ReadWrite);                
        }
        return f;
    }

In this case, catching the exception is (very) marginally safer, as the file could get deleted between the time you check for its existence and the time you open it.

There are reasons not to do this, sure. In .NET, exceptions are computationally expensive, so you want to avoid anything that throws a lot of them. (In Python, where exceptions are cheap, it's a common idiom to use exceptions to do things like break out of loops.)

But that's ignoring a specific exception. This code:

catch
{
}

is inexcusable.

There's no good reason not to catch the specific typed exception that the code in the try block is going to throw. The first reason that the naive developer gives for catching exceptions irrespective of type, "But I don't know what type of exception might get thrown," is kind of the answer to the question.

If you don't know what type of exception might get thrown, you don't know how your code can fail. If you don't know how your code can fail, you have no basis for assuming that it's OK to just continue processing if it does.

Yes, its acceptable (unavoidable, necessary) in certain circumstances per Maxim's post. That doesnt mean you have to like it. 2106 violations is probably WAY too many and at the least they should have added a comment in the catch block as to why it was ok to swallow this exception.

@Dustin Its bad practice to show any exception details to a public user (stack traces, line numbers, etc). You should probably log the exception and display a generic error.

When it comes to Exceptions, there are always exceptions.

It depends on the framework. Badly implemented frameworks might actually require this. I recall a hack in VB6 where there was no way to determine whether a collection contained an element. The only way was to try to retrieve the element and swallow the error.

I think the original question has been well answered, but one thing I'd like to add is that if you think these outsourced/contracted developers produced poor quality work, you should make sure the right people at your company know about it.

There may be a chance that it can be sent back for rework, that payment can be partially withheld, or that the same firm won't be used again. If your company uses contractors again, they might find a way to build quality requirements into the agreements, assuming that's not alredy there.

If this was in-house work, there would be consequences to the team/individual that produced substandard work. Maybe that would just mean that they have to work nights and weekends to fix it, but they'd be on the hook for it one way or another. The same should apply to contractors, possibly even more so.

Just be careful to explain your position professionally and with a focus on what's best for the company/product. You don't want it to seem like you're just complaining, or that you have some kind of political objection to outsourcing. Don't make it about you. Make it about cost, time to market, customer satisfaction, etc. You know, all those things that management types care about.

I wonder about this specific one a lot.

Connection con = DriverManager.getConnection(url, "***", "***");

try {
    PreparedStatement pStmt = con.prepareStatement("your query here");

    ... // query the database and get the results
}
catch(ClassNotFoundException cnfe) {
    // real exception handling goes here
}
catch(SQLException sqle) {
    // real exception handling goes here
}
finally {
    try {
        con.close();
    }
    catch {
        // What do you do here?
    }
}

I never know what to do in that last catch in the finally block. I've never seen close() throw an exception before, and it's so unlikely that I don't worry about it. I just log the exception and move on.

Unless your catch code will either

1. Log the exception
2. Repackage the exception into another exception that matches the same abstraction. and throw again
3. Handles the exception the way you see suitable

You can ignore the exception, but at least mention the expected exception in the method docs, so the consumer can expect and handle if necessary

To take an example from the Java world where it's OK to ignore an exception:

String foo="foobar";
byte[] foobytes;

try
{
    foobytes=foo.getBytes("UTF-8");
}
catch (UnsupportedEncodingException uee)
{
    // This is guaranteed by the Java Language Specification not to occur, 
    // since every Java implementation is required to support UTF-8.
}

That said, even in situations like this, I'll often instead use

...
catch (UnsupportedEncodingException uee)
{
    // This is guaranteed by the Java Language Specification not to occur, 
    // since every Java implementation is required to support UTF-8.
    uee.printStackTrace();
}

If the virtual machine is going to be mad/spec-breaking, there's little I can do about it, but with the stack trace, I at least get to notice when it started its descent into madness...

That's a really bad thing to be doing.

While there are valid reasons you might want to ignore exceptions - if it's expected in some way, and there's no need to do anything about it - however doing it 2000 times seems like they just want to sweep their exceptions under the rug.

Examples of where it's OK to swallow exceptions might be probing for things... you send off a message to some device or module, but you don't care if it actually gets there.

The following only applies to languages that have checked exceptions, e.g. Java:

Sometimes a method throws a checked Exception that you know won't happen, e.g. some java APIs expect an encoding name as a string and throw a UnsupportedEncodingException if the given encoding isn't supported. But usually i pass a literal "UTF-8" that i know is supported so I could theoretically write an empty catch there.

Instead of doing that (empty catch) I usually throw a generic unchecked exception wrapping the "impossible" exception, or I even declare a class ImpossibleException that i throw. Because my theory about that error condition being impossible might be wrong and in that case I wouldn't want the exception to be swallowed.

I like to let almost all of my exceptions bubble up to an application handler where they are logged and a generic error message is displayed to the end user. But the caveat here is that there should not be very many exceptions that actually occur. If your application is throwing many exceptions, then there's probably something wrong or something that could have been coded better. For the most part, I try to make sure that my code checks for exceptional cases in advanced because generating exceptions is expensive.

As an aside, outsourcing coding is generally a bad idea. From my experience, usually they are consultants who are only in it for the paycheck and have no stake in the success of the project. Also, you surrender to their -lack of- coding standards (unless you included that in the contract).

Think of it this way - if you are expending the CPU cycles to catch the exception but then swallowing, you are ignoring a potential issue and wasting CPU. As many have said the application should not be throwing that many exceptions unless you have something poorly constructed.

We have an application that does a lot of processing on behalf of other applications, where you insert some job (collection of config) into a database and the app will take it and run it at the appropriate time. We tend to swallow a lot of exceptions in that application, because even if Job1 dies in a horrifying fashion with a catastrophic error, we want the app to stay alive to take a stab at processing Job2.

I think the best rule of thumb is only ignore an exception if you're completely aware of what the exception means and the possible ramifications of it. In the case of some isolated module that doesn't affect the rest of your system I think it would be okay to just catch the generic Exception as long as you know nothing bad happens to anything else.

IMO it's easier to know the ramifications in Java since each method is required to declare all exceptions it can throw so you know what to expect, but in C# an exception can be thrown even if it isn't documented, so it's hard to know all the possible exceptions that can be thrown by a method, and lacking that knowledge it is usually a bad idea to catch all.