50

I have read a few of the other questions regarding C# Exception Handling Practices but none seem to ask what I am looking for.

If I implement my own custom Exception for a particular class or set of classes. Should all errors that relate to those classes be encapsulated into my exception using inner exception or should I let them fall through?

I was thinking it would be better to catch all exceptions so that the exception can be immediately recognized from my source. I am still passing the original exception as an inner exception. On the other hand, I was thinking it would be redundant to rethrow the exception.

Exception:

class FooException : Exception
{
    //...
}

Option 1: Foo encasulates all Exceptions:

class Foo
{
    DoSomething(int param)
    {
        try 
        {
             if (/*Something Bad*/)
             {  
                 //violates business logic etc... 
                 throw new FooException("Reason...");
             }
             //... 
             //something that might throw an exception
        }
        catch (FooException ex)
        {
             throw;
        }
        catch (Exception ex)
        {
             throw new FooException("Inner Exception", ex);
        }
    }
}

Option 2: Foo throws specific FooExceptions but allows other Exceptions to fall through:

class Foo
{
    DoSomething(int param)
    {
        if  (/*Something Bad*/)
        {
             //violates business logic etc... 
             throw new FooException("Reason...");
        }
        //... 
        //something that might throw an exception and not caught
    }
}
  • 1
    just a quick note FooException should extend ApplicationExcetpion as best practice. – David Waters Jan 21 '11 at 17:32
  • 13
    @DavidWaters - You would expect that to be the case, but please see: [link](http://msdn.microsoft.com/en-us/library/system.applicationexception.aspx) where it states, "*If you are designing an application that needs to create its own exceptions, you are advised to derive custom exceptions from the Exception class. It was originally thought that custom exceptions should derive from the ApplicationException class; however in practice this has not been found to add significant value*." – Anthony Walsh Jul 08 '12 at 10:38
  • 4
    ApplicationException has been acknowledged as a bit of a mistake and it is better to extend System.Exception as per http://msdn.microsoft.com/en-us/library/vstudio/ms229064(v=vs.100).aspx – krystan honour Oct 03 '13 at 10:40

8 Answers8

57

Based on my experience with libraries, you should wrap everything (that you can anticipate) in a FooException for a few reasons:

  1. People know it came from your classes, or at least, their usage of them. If they see FileNotFoundException they may be looking all over for it. You're helping them narrow it down. (I realize now that the stack trace serves this purpose, so maybe you can ignore this point.)

  2. You can provide more context. Wrapping an FNF with your own exception, you can say "I was trying to load this file for this purpose, and couldn't find it. This hints at possible correct solutions.

  3. Your library can handle cleanup correctly. If you let the exception bubble, you're forcing the user to clean up. If you've correctly encapsulated what you were doing, then they have no clue how to handle the situation!

Remember to only wrap the exceptions you can anticipate, like FileNotFound. Don't just wrap Exception and hope for the best.

Tesserex
  • 17,166
  • 5
  • 66
  • 106
  • 14
    I think the important part of this answer is if you are providing more context. If you are not providing this additional context then let the exception through untouched. – btlog Jan 21 '11 at 16:41
  • I agree that providing context is critical, but I think point 3 is just as important, even if it's a totally unrelated reason. – Tesserex Jan 21 '11 at 16:43
  • I don't see throwing a new exception wrapping another exception as cleaning up, it is still passing the buck to the caller. – btlog Jan 21 '11 at 16:46
  • Somethings cannot always be handled. If the database becomes unavailable or connection is lost, I will clean up the objects I am using, but the exception should be handled by those implementing the library. –  Jan 21 '11 at 16:48
  • 1
    By cleaning up, I mean disposing of resources and such. If I open a file, and then have an exception while reading it, the user of my code can't possibly close my file. I need to at least have my own finally block around it to do that. – Tesserex Jan 21 '11 at 16:48
  • 1
    I disagree with point 3. Cleanup is addressed with a finally block, not with a catch. Points 1) and 2) are correct though. – H H Jan 21 '11 at 16:49
  • That is true, so it is somewhat distantly related. But I took it to mean choosing between doing *nothing* and doing *something* with it. If he chooses option 2 and doesn't put any try block at all around his code, then he certainly isn't cleaning it up either. – Tesserex Jan 21 '11 at 16:50
  • 1
    @Tess: The library should put a _finally_ block around code that manages a resource. That is totally separated from catching. – H H Jan 21 '11 at 16:52
  • [It's now possible in .NET 4.5](http://blogs.microsoft.co.il/blogs/sasha/archive/2011/10/19/capture-transfer-and-rethrow-exceptions-with-exceptiondispatchinfo-net-4-5.aspx) to preserve the original trace with the `ExceptionDispatchInfo` class while providing the contextual clarity of a custom Exception. – Travis Watson Mar 22 '13 at 21:47
  • Thank you, @Tesserex. But what about, for example, `ArgumentException`? Consider a method `SendMail(string message)`, throwing a `SendMailException`. If I want to validate method argument, should I wrap `ArgumentException` to the `SendMailException` as well? – odinserj Dec 12 '13 at 13:48
  • 1
    @odinserj It depends if the exception happens on the public side of your library or internally. If the user passed in a bad argument, give them the `ArgumentException`. They'll understand it. If something your library did down in the depths caused it, you would wrap it. Then again, what you should really do is fix it because it means your library has a bug. – Tesserex Dec 12 '13 at 15:06
  • I know this is old, but I *completely* disagree with @Tesserex's assesment. "Clean up" should be done in a "Finally" block (which does not require a "Catch" at all. Do *not* change the data type of exceptions that you are sending out from a reusable library. If the caller wants to handle the exception, you've just made his life 100x more difficult. The whole point of having different exception classes is so that they can be handled differently. "Wrapping" exceptions removes the capability to cleanly handle different types of exceptions. – Mike U Jan 21 '16 at 15:55
20

Have a look at this MSDN-best-practises.

Consider to use throw instead of throw ex if you want to re-throw caught exceptions, because on this way the original stacktrace keeps preserved(line numbers etc.).

Tim Schmelter
  • 450,073
  • 74
  • 686
  • 939
  • +1 In this particular case I am not trying to handle the exception, just rewrap the exception to to notify down the stack that it came from my class. But definitely worth noting. –  Jan 21 '11 at 16:42
  • 1
    @snmcdonald: Because you are not trying to handle the exception i mentioned this, you also could re-throw the same exception up to a "global" exception handling class. And then you should know that it's possible to keep the original stack trace. My answer was more a general suggestion/hint. – Tim Schmelter Jan 21 '11 at 16:48
  • 3
    @TimSchmelter: Good information, but this is not a relevant answer for this question. It appears you stopped reading after the headline. Nothing is gained with your suggestion in this scenario; you caught the exception for no reason (your `throw;` is equivalent to OP's Option 2 fall-through since he's not doing any additional processing). – Travis Watson Mar 22 '13 at 21:43
  • The issue, here, is that the op's entire thinking about exception handling is flawed. He's not actually handling the exception. "Knowing that it came from my library" is accomplished by looking at the stack trace, not by obfuscating the actual exception type with a wrapper. "Clean up" code should be done in a "Finally" block, which does not require a "Catch" block to be present. – Mike U Jan 21 '16 at 15:57
6

I always add a couple of properties when creating a custom exception. One is user name or ID. I add a DisplayMessage property to carry text to be displayed to the user. Then, I use the Message property to convey technical details to be recorded in the log.

I catch every error in the Data Access Layer at a level where I can still capture the name of the stored procedure and the values of the parameters passed. Or the inline SQL. Maybe the database name or partial connection string (no credentials, please). Those may go in Message or in their own new custom DatabaseInfo property.

For web pages, I use the same custom exception. I'll put in the Message property the form information -- what the user had entered into every data entry control on the web page, the ID of the item being edited (customer, product, employee, whatever), and the action the user was taking when the exception occurred.

So, my strategy as per your question is: only catch when I can do something about the exception. And quite often, all I can do is log the details. So, I only catch at the point where those details are available, and then rethrow to let the exception bubble up to the UI. And I retain the original exception in my custom exception.

DOK
  • 32,337
  • 7
  • 60
  • 92
3

The purpose of custom exceptions is to provide detailed, contextual information to the stacktrace to aid in debugging. Option 1 is better because without it, you don't get the "origin" of the exception if it occurred "lower" in the stack.

Dave Swersky
  • 34,502
  • 9
  • 78
  • 118
1

if you run the code snippet for 'Exception' in Visual Studio you have a template of a good practice exception writing.

Felice Pollano
  • 32,832
  • 9
  • 75
  • 115
1

Note Option 1: your throw new FooException("Reason..."); won't be caught as it's outside try / catch block

  1. You should be only catching exceptions that you want to process.
  2. If you're not adding any additional data to the exception than use throw; as it won't kill your stack. In Option 2 you still might do some processing inside catch and just call throw; to rethrow original exception with original stack.
Jakub Konecki
  • 45,581
  • 7
  • 87
  • 126
  • 2
    I think his first throw in Option 1 isn't meant to be caught. It's just an example of another case where he can throw his own exception. "Something bad" may just be that param was negative, for example. – Tesserex Jan 21 '11 at 16:42
1

The most important thing for code to know when catching an exception, which is unfortunately completely missing from the Exception object, is the state of the system relative to what it "should" be (presumably the exception was thrown because there was something wrong). If an error occurs in a LoadDocument method, presumably the document didn't load successfully, but there are at least two possible system states:

  1. The system state may be as though the load were never attempted. In this case, it would be entirely proper for the application to continue if it can do so without the loaded document.
  2. The system state may be sufficiently corrupted that the best course of action would be to save what can be saved to 'recovery' files (avoid replace the user's good files with possibly-corrupt data) and shut down.

Obviously there will often be other possible states between those extremes. I would suggest that one should endeavor to have a custom exception which explicitly indicates that state #1 exists, and possibly one for #2 if foreseeable but unavoidable circumstances may cause it. Any exceptions which occur and will result in state #1 should be wrapped in an exception object indicating state #1. If exceptions can occur in such a fashion that the system state might be compromised, they should either be wrapped as #2 or allowed to percolate.

supercat
  • 77,689
  • 9
  • 166
  • 211
0

Option 2 is best. I believe best practice is to only catch exceptions when you plan to do something with the exception.

In this case, Option 1 just is wrapping an exception with your own exception. It adds no value and users of your class can no longer just catch ArgumentException, for example, they also need to catch your FooException then do parsing on the inner exception. If the inner exception is not an exception they are able to do something useful with they will need to rethrow.

btlog
  • 4,760
  • 2
  • 29
  • 38
  • But it's unlikely that a caller would want to intercept ArgumentException. Catching FooException(s) is likely to make more sense. – H H Jan 21 '11 at 16:44