views:

220

answers:

7

Hi,

why (if at all) is this a bad idea ?

class Program
{
  static void Main(string[] args)
  {
     try
     {
        throw new NotImplementedException("Oh dear");
     }
     catch (Exception ex)
     {
        throw NewException("Whoops", ex);
     }
  }

  // This function is the salient bit here
  public static Exception NewException(String message, Exception innerException)
  {
     return Activator.CreateInstance(innerException.GetType(), message, innerException) as Exception;
  }
}

The important bit here is that the function creates an exception of the same type as the "innerException".

I'm thinking... "Oh... an exception has occurred. I can't actually handle it here, but I can add some additional information, and re-throw. Maybe another handler, higher up the call chain can handle it."

A case in point might be some sort of SQL error. I might not be able to handle the exception at the point of calling, but might wish to add some additional "context" information like "I was calling this, and passing that".

It seems like it might be useful to pass back up the call chain an exception of the type that was originally raised, as opposed to "Exception" or "ApplicationException". I know I could create my own custom exception classes, but it seems that it adds nothing much when you already have a nice specific exception.

Of course, I might be wrong. It might be a very useful thing to do... but a little voice is suggesting not.

----- edit -----

For the sake of debate, consider the effects of the following two functions (using the code above):

This... seen all too often:

  static int SalesTotal(int customerNumber)
  {
     try
     {
        throw new DivideByZeroException(); // something you didn't expect
     }
     catch (Exception ex)
     {
        throw new ApplicationException("Unable to calculate sales for customer " + customerNumber, ex);
     }
  }

versus this...

  static int SalesTotal(int customerNumber)
  {
     try
     {
        throw new DivideByZeroException(); // something you didn't expect
     }
     catch (Exception ex)
     {
        throw NewException("Unable to calculate sales for customer " + customerNumber, ex);
     }
  }
+2  A: 

Don't catch it at all and just let it bubble up - there's no point reinventing the wheel.

Not only is your method not intuitive for somebody else coming in, but you're also going to lose the original stack trace from the first exception.

Paddy
In the original example, the original stack is not lost.
Ross Watson
+10  A: 

all exceptions have a Data property that you can add additional data to. There is no need to create a new exception. Just catch the existing exception and add your information and then just rethrow the exception.

This way you get your cake and eat it too. :)

http://msdn.microsoft.com/en-us/library/system.exception_members(v=VS.90).aspx

Khalid Abuhakmeh
Just make sure to rethrow the exception with `throw;` instead of `throw ex;`; otherwise, the stack trace is lost.
Heinzi
...and there can be trouble even when using `throw;`: http://weblogs.asp.net/fmarguerie/archive/2008/01/02/rethrowing-exceptions-and-preserving-the-full-call-stack-trace.aspx.
Heinzi
Hmmm... the Data collection might have some possibilities - but plenty of scope to be rather vague in its usage.
Ross Watson
+3  A: 

I think you should create your own custom exception type. What kind of information are you going to add? How is the catcher of the exception going to know it has extra information? If you use a custom type, they will have extra properties/methods to view or call.

Bryan
I agree, just create a custom exception type, and possibly add the original exception as InnerException of the new one.
digEmAll
A: 

It can make sense in some cases. If you consider MyClass.MyFunction the public "outside" of the assembly, then what happens inside it is a black box to calling code. It can therefore be more reasonable to have that as the point that the exception happened as far as the calling code goes, and the most reasonable type of exception could happen to be the same type as that caught.

I would be cautious though. In most cases of this happening, either you should have been able to catch that the exception was going to happen, and thrown pre-emptively (if you are going to throw, the sooner the better) or else the exception you are throwing isn't really the right type (if your code doesn't find a file it needs that's a FileNotFoundException to you, but if the files are an implementation detail, then it's not a FileNotFoundException to the calling code), or else the original exception will make perfect sense and it should just be allowed to call through or rethrown with throw;.

So, not always a bad idea, but bad often enough that it's always a suspicious thing to see.

Jon Hanna
+4  A: 

Creating a new exception type isn't a good option for a general method like this, since existing code will be unable to react to a specific error. (Translation exceptions at API boundaries cane be useful, though).

Creating a new exception of the same type seems perilous. Does the CreateInstance overload you're using copy all fields from the innerException to your new outer exception? What if an exception handler higher up the stack depends on parsing the Message property? What if the exception constructor has side effects?

IMHO, what you're really doing is logging, and you'd probably be better off actually doing logging and a re-throw.

snemarch
Some valid points. I imagine that CreateInstance just invokes the type's constructor, with a string and an (inner) exception. Also, I would be even more wary if I found code that relied on the format of an exception's message property.
Ross Watson
Parsing exception messages definitely isn't pretty, but I've seen it done in Java code dealing with SQL, in order to react to specific constraint violations.
snemarch
Actually... now you mention it, I think I've done the same (in the same circumstances but C#) myself.
Ross Watson
A: 

IMHO, it would probably be good to have a few custom exception types, with one hierarchy for exceptions which say "This operation failed, but the system state is as though it had never been attempted", another for exceptions that say "The operation failed, and the system state has been disturbed, but it can probably be unwound", and another for "The operation failed, and unwinding failed; the system state cannot be trusted." In some contexts, one class of exception may be caught and rethrown as another (e.g. if an operation which was supposed to finish restoring system state fails, even if from the point of view of that operation nothing was disturbed, the failure to restore the state may leave it disturbed; conversely, if an operation left a system state disturbed but a catch handler restored the state, it could rethrow as an "operation failed but system state is undisturbed".

I dislike very much the idea of trying to create exception instances that blindly match thrown exception types, since the thrown exceptions may have fields which handlers will expect to be populated. I'm not sure why Microsoft made exceptions "almost" immutable. It would be much nicer if exceptions were required to be immutable and support cloning with whatever semantics conveyed immutability.

supercat
A: 

Only catch exceptions that you handle, for instance you do not want a SqlException to be thrown from your data layer to your business layer (catch it, try ot handle it and throw a DataLayerException or something like that instead).

jgauffin