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?

5

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
}
  • 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

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.