Is it okay that I sometimes sink my exceptions?

Question

I have a best practices question. I realize this is subjective but wanted to ask people smarter than me if this is a common programming practice.

If you have a method of something NON-CRITICAL that you don't want to interfere with the important functioning of your application, is it common to use an error sink like this?

Try 
    'do stuff.  not important if it fails.

Catch ex as exception
    'sink.  do nothing.
End Try

If you were thinking of hiring me and you were reading some of my code and saw this...would you?

Seth

EDIT Wow! Thanks for your answers. I think the consensus is that should never be done or it should be very rare.

I thought I would give you context for this question. First, I have a strong familiarity with the Karl Sequin article and have followed that pattern for years.

But today on the project I was working on, I was working through the change list and was faced with the addition of a simple feature. (If you care to know...it is adding context menu support to a Rich Text Box.)

The attached note said, "if it takes longer than 15 mins...drop it."

So I am faced with adding what is a potentially useful feature but the don't really have the time to test that it won't break working features. For the record, our exception handler for this system DOES have a mechanism for handling and sinking or logging these errors. But what if I was working on a system that did not have a robust error handling system. Would it be okay to add this feature and if an error occurs...nothing is really lost.

That was my thinking. But I have taken your message to heart...that basically this is a bad idea.

Seth

Answer 1

I would say "Don't do it" - not like that.

First of all, try refactoring the "noncritical" code so that it doesn't throw an exception.

If you are unable to do that, at the very least don't blindly catch Exception. Only catch the exceptions that you expect it to throw (and log them somewhere!) - anything else is something you need to be made aware of.

Answer 2

Yes, it is common, but in general it shouldn't be done.

There are exceptions like OutOfMemoryException which are better not caught, unless you catch them to attempt to terminate your application gracefully.

In the majority of cases, swallowing System.Exception or System.SystemException will inevitably hide further run-time problems.

Answer 3

Personally I see this as being incredibly bad practice.

It is (unfortunately) one of the things I always look for when reviewing code, and the questions I ask when I find empty catch blocks or catch blocks that essentially swallow exceptions are:

1. At this moment in time are you 100% sure that this exception will never matter?
2. Are you also 100% certain that any exception caught here in the future will never matter, regardless of how the code base develops?
3. Even if 1 and two are both true, is it really so hard to put some logging here?

The most important thing for me is the logging - good logging of exceptions, and good tracing of program execution are fundamental to the design of code that can be safely modified over time, and that evolves into a stable system that users have faith in.

Beyond that, a good practise is to only catch specific exceptions, and to let all other exceptions bubble up the stack. Blindly handling exceptions as a way of handling errors is never correct.

Answer 4

Sometimes I find that developers would rather catch and swallow an exception instead of writing correct error handling code.

There's some code, if throws an exception, they catch the exception, try something else, and if that throws an exception, do something else, etc. Exceptions are objects and are created when they are thrown. If you can, write the correct handling code to deal with an error and only use exceptions when you can't continue for whatever reason.

As for swallowing exceptions, I'd be lying if I said I'd written code which didn't. It's not a best practice and not to mention extremely hard to debug when your program behaves poorly. I've been bit by them too.

Answer 5

I think the best way to answer this is to talk about cases where it IS acceptable to do it. In most cases it is NOT safe. In those cases where it is safe, it is usually best to at least spit out some information about it in the warn, debug, or trace log. I can think of only a few cases where it is safe:

You expect a very specific exception, and know that you can recover from it:

try {
 Integer.Parse(someString);
}
catch (ParseException pe) {
 //In this particular use case, if the string doesn't parse, 
 //I don't need to do anything. Logging a warning might be a
 //good idea, in some cases
}

You are making cleanup code safe:

beginDatabaseTransaction();
try {
 doSomeDatabaseOperation();
}
catch (Exception e) {
 log.error(e); //log the original exception
 try {
  rollbackDatabaseConnection();
 }
 catch(Exception e2) {
  //You may ignore this one-off exception, though
  //I would strongly consider logging it
 }
 throw; //rethrow the original exception and let it propagate
}

(This is by no means a best practice pattern for database operations; just a contrived example.)

In our production system, we have had more than one case where overzealous exception trapping caused much more damage than it did good. Be careful with this.

One other thing: If you don't except a specific error to be thrown, you usually should not be trying to catch one.

Most exceptions are there to help you, the developer, find and diagnose mistakes in your code. They are designed to make the program (or the current operation) terminate, to prevent unpredictable behavior (like trying to load all of the records in your the database, because you weren't able to construct the WHERE clause for your SQL statement.) When you catch an unknown exception, you are concealing the mistake from your currently-running code and making it possible for the code to behave unexpectedly, instead of just terminating.

Answer 6

.if using vs2008 you could at least send them to the debug window

 System.Diagnostics.Debug.WriteLine("exception in method - my method -: "+ex.message);