0

Consider the following code:

public class ExceptionManager
{
    public static void TreatException(Exception ex)
    {
        if (ShowAndContinue(ex))
            // display a user-friendly message on what happened and let the app run.
        else
            throw ex;
    }

    // (more code)
}

(Please note that the above is over-simplified so that the focus is on my actual concern)

IMHO I think the way the exception is thrown is wrong because stacktrace information is lost. The preferred way (again, IMHO) to write the throw line would be this:

throw new UntreatableErrorException("Manager could not treat the exception.", ex);

This way the original exception's stacktrace is preserved.

However, a colleague of mine is arguing that doing so would mean losing the ability to catch the original exception lower in the call stack, because catch works with exception type and hence an exception trap might be set to catch the original exception but not the UntreatableErrorException. Therefore it's better to throw back the original exception instance, even if it means losing the stacktrace.

Which one of us is right here on what should be the best practice?

6

You are both right.

You don't want to lose the stack trace, but you may want to catch the lower level exception.

Don't wrap the exception if it isn't adding information.

I would call your ExceptionManager and TreatException a bit of a code smell - they are trying too hard to centralize exception handling.

To re throw without losing the stack trace:

public static bool ContinueAfterException(Exception ex)
{
    if (ShowAndContinue(ex))
    {
      // display a user-friendly message on what happened and let the app run.
      return true;
    }

    return false;
}

Elsewhere:

try
{ 
  // some exception throwing code
}
catch(Exception ex)
{
    if(!ContinueAfterException(ex))
        throw; // no ex
}
| improve this answer | |
  • I do agree on the code smelling. Unfortunately, I have little to no authority over it. :) Right now I'm just trying to convince the people behind it that they should AT LEAST be wrapping the exception, since it's thrown from a higher layer which they know nothing about. About "not adding information", would you mind clarifying? The way I see it, while the current implementation does not need to add information, it shouldn't take away any of it either (original stacktrace is information, isn't it?). – Crono Nov 26 '14 at 18:40
  • @Crono - it is hiding information (the original exception is now in the InnerException of the thrown one. If your new exception narrows down the issue (ArgumentException => DivideByZeroException, for example) or adds a good description, it doesn't really add much. – Oded Nov 26 '14 at 19:06
  • @Crono An exception has three information channels: (1) the type of the exception (so that I can catch (SpecificException e) somewhere), (2) the message which provides context for debugging, and (3) the stack trace which allows the cause to be located. Wrapping hides the original type and substitutes an obfuscating message. Every exception was an UntreatableException before it was caught, this is implied by letting an exception bubble up through the call chain. Oded has shown you a solution how to rethrow if you can't fix the problem – but without loosing or hiding any information channel. – amon Nov 26 '14 at 19:11
  • @Oded I'm sorry, I'm confused here. InnerException is a public property on the base Exception class. How is it hiding the original exception to wrap it inside this property? Also, look at TypeInitializationException or TargetInvocationException: these classes hardly adds any information to exceptions, and yet they're used as wrappers. Why? – Crono Nov 26 '14 at 19:38
  • @Crono - what do you mean how does it hide it? When you catch an exception and look at its message and details, the actual exception is not in the type of your wrapped exception, it isn't in the message. It is inside a property. It is not out there as it would be if you let it bubble up. – Oded Nov 26 '14 at 19:40
0

What I see here, that you want to catch a subset of possible errors and you rethrow what you did not want to catch. The right solution would be not catching what you don't want to catch, so you won't need to rethrow it.

I am not sure what C# allows, because I never worked with that language, but my guess would be that wrapping your treatable exceptions with TreatableException or adding a Treatable interface to them and changing your code to TreatException(Treatable ex) would work in most of the cases.

| improve this answer | |
  • IIRC, this issue came up after reviewing the code from a 3rd party. Hence I have no control over it and the vendor won't change its behavior because obviously it may end up breaking existing apps. I was hoping (again IIRC, this was six years ago :) ) I could at least convince the vendor to have a way to initialize its component so that it would throw a wrapped exception so that I would still have the original stacktrace. – Crono Feb 14 at 18:34

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.