How to deal with exceptions

Question

My technical lead insists on this exception mechanism:

try
{
    DoSth();
}
catch (OurException)
{
    throw;
}
catch (Exception ex)
{
    Util.Log(ex.Message, "1242"); // 1242 is unique to this catch block
    throw new OurException(ex);
}

1242 here is an identifier of the catch method which we handle an exception other than OurException. Every catch block in the project must have a unique identifier so we can know where the exception occurred by looking at the logs.

For every method, we have to catch OurException and throw it. If an other type of exception is thrown, we have to log it and mask it by OurException before rethrowing it.

Is this a reasonable approach? If there are any, what are the better alternatives?

Edit: I've been told the stack trace does not produce meaningful results in release mode. Are you suggesting catching and throwing generic exceptions is OK?

Edit2: Thank you all. I used your answers as part of my argument against this but I've been told you are not experienced enough and do not know how to deal with real life situations. I have to go this way.

Answer 1

I think that having a hard coded number on the throw line is not a good practice, how do you know whether that number is being used or not?, or, which is the next error number to throw? Maybe you can have, at least, listed on an enum... not sure.

Answer 2

Everything looks right except for that weird number.

The stack trace will contain the information you need.

Answer 3

I would think that using the stack trace would be much more intuitive than any identifier.

As far as the custom exception, why not do this?

try
{
DoSth();
}
catch(Exception ex)
{
Util.Log(ex.Message, ex.StackTrace);
if(ex is OurException) throw ex;
else throw new OurException(ex); // use the original exception as the InnerException
}

Also, I'm not sure why you'd want to rethrow an exception after it's been handled - can you explain the reasoning behind that?

@Ali A - A very valid point, so allow me to rephrase - why rethrow the exception instead of finishing the handling of it right here?

EDIT:

Instead of rethrowing, why not do this?

try
{
DoSth();
}
catch(Exception ex)
{
Util.HandleException(ex);
}

Util.HandleException:

public static void HandleException(ex)
{
Util.Log(ex); // Util.Log should log the message and stack trace of the passed exception as well as any InnerExceptions - remember, than can be several nested InnerExceptions.

// Any additional handling logic, such as exiting the application, or showing the user an error message (or redirecting in a web app)
}

That way, you know every exception only gets logged once, and you're not throwing them back into the wild to wreak any additional havoc.

Answer 4

You can also look into the Exception Handling Application block.

I have used it in a few projects and it is very useful. Especially if you want to later change how your exception handling works, and what information to capture.

Answer 5

Logging the stacktrace would be a lot better than the hardcoded number. The number tells you nothing about what routine called this one.

Also the number is a pain to manage, and error-prone at that.

edit: it is true that the stacktrace doesn't always produce meaningful results in release mode, but I think the same can be said of this hardcoded number scheme! :-)

With the language you're using, does the compiler provide macros with the current filename and line number? That would be better than trying to roll a unique number yourself. And, if you can't do that automatically, do it manually, or come up with some scheme to guarantee uniqueness that doesn't require some central allocation of numbers!

Answer 6

Seems to me that the stacktrace is a better way to handle this. What happens if you mistakenly reuse a number?

As far as whether this is a good idea or not, in the absence of any more information, I'd tend to say no. If every method invocation is wrapped in a try/catch block it will be very expensive. Generally, exceptions fall in to two camps -- those you can reasonably expect and deal with and those that are really bugs (or at least not successfully anticipated). Using a shotgun approach to catching all exceptions seems to me like it just might do more to hide the latter than help get them resolved. This would be especially true if at the top level you caught all exceptions so that the only record you have is the log message.

Answer 7

I have two types of exceptions: repairable exception and fatal exception. If some object throw repairable exception, this is mean that error occur but object is not damaged and can be used over again. If some object throw fatal exception, this is means that object state is damaged, and any attempt to use object will lead with new error.

Update: all exceptions might be handled as soon as possible, and as close to error source as possible. For example, if object, stored in collection throws the fatal exception, exception handler just remove this object from collection and delete it, not all entire collection of objects.

Answer 8

I fail to see how this is helpful. You can already know where the exception came from with the stack trace, and you are adding an unnecessary level of complication.

Answer 9

The drawbacks:

1. That number doesn´t inspire much confidence, stack trace is much better

2. Why have 2 catch blocks if your custom exception can be handled on the "catch(Exception ex) "?

Answer 10

Having the OurException is kinda weird. Usually, you want to have specialized catch blocks and then the last one, the one that catches a generic Exception is where you do your logging:

try 
{
    DoSomething();
}
catch (DivideByZeroException)
{
    // handle it here, maybe rethrow it, whatever
}
// more catch blocks
catch (Exception)
{
    // oops, this is unexpected, so lets log it
}

But what your doing will work. I do believe the 1242 should go though. Here's a method to print the method, filename, and line number you could use instead. I haven't tried it myself but it looks good:

    [Conditional("DEBUG")]
    public static void DebugPrintTrace()
    {
        StackTrace st = new StackTrace(true);
        StackFrame sf = st.GetFrame(1); // this gets the caller's frame, not this one
        Console.WriteLine("Trace "
            + sf.GetMethod().Name + " "
            + sf.GetFileName() + ":"
            + sf.GetFileLineNumber() + "\n");
    }

Answer 11

If you're going to use this pattern, then I'd like to see some more information added to OurException when it's created than merely the exception that it wraps, such as as description of what was DoSth() supposed to do, along any relevent context infomation such as passed parameters. Then, when you come to catch OurException for real, you've got some useful information of what went wrong, beyond that which is already contained in the wrapped exception.

Answer 12

I think this is a bad pattern, because you have the information about the whole call stack readily available in the exception, there is no need to pseudo-handle the exception by logging and rethrowing it. Only catch an exception at a place where you really can do something about the problem.

Answer 13

From what I understand, the purpose of exceptions are to propagate unexpected errors. If you catch it close enough to the method that throws it, you are more in the context of how to handle it.

Your example is to catch an unexpected error, and rethrow that up the chain. This is not handling the exception, but wrapping it into your own.

But your wrapping does not seem to add any more value to the exception, but might even clutter things.

An extreme example:

void a() {
  try {
    c();
  } catch(MyException1 ex) {
    throw;
  } catch(Exception ex) {
    log(ex);
    throw new MyException1(ex);
  }
}

void b() {
  try {
    a();
  } catch(MyException2 ex) {
    throw;
  } catch(Exception ex) {
    log(ex);
    throw new MyException2(ex);
  }
}

Notice how that, in the earlier example, the original exception is logged twice. And it is wrapped in two exceptions. When you log the same exception a few times it becomes harder to trace (since the log file grows rather big).

Of course this might be an extreme example, but I find it hard to believe that your entire application has only one type of exception in use. It does not describe sufficient all the different types of errors that might happen.

I personally prefer to log the exception only at the catch block where I handle it. Anywhere else might just create duplicated logging.

Answer 14

My application is built in release mode, and i use the Exception Handling Application Block, so i never have any catch ex blocks of code, it is caught by the EHAB, this itself writes to a log file with all the info i need; stack trace etc, time, etc.

the only problem with this is if someone has put

catch ex
{
    //do stuff
    throw ex;
}

as this will wipe out the stack trace.