Exception handling no-no’s

Just spotted this in a project I’m working on:

public static string GetExceptionAsString(this Exception exception)
{
	return string.Format("Exception message: {0}. " + Environment.NewLine +
 		"StackTrace: {1}. {2}", exception.Message, exception.StackTrace,
		GetAnyInnerExceptionsAsString(exception));
}

Writing code like this should be a shooting offense.

But wait, there’s more! Check out its usage:

try
{
	...
}
catch (Exception ex)
{
	log.InfoFormat("Error occured:{0}.", ex.GetExceptionAsString());
}

Agh!

Code like this demonstrates a complete misunderstanding of CLR exception basics — inner exceptions and formatting — and also log4net. All that hand-rolled formatting could simply be replaced with:

  • GetExceptionAsString() -> Exception.ToString()
  • ILog.InfoFormat(format, args) -> ILog.Info(message, exception)

6 thoughts on “Exception handling no-no’s

  1. Hi,

    In general I strongly agree with one exception. I have seen one situation which I think made this kind of code acceptable. Looping over a collection of subscribers where exception of any sort in any one of them should not interfere with the rest.

    What do you think?

    Best regards,

    Petar

  2. I find this post to be entirely pointless (except the perhaps in regards to log4net?). There’s nothing wrong with that code you posted originally, I’ve actually written nearly the same code for writing the full message, stacktrace and all inner exception messages to various data stores.

    And I don’t write bad code.

  3. @ Chris: and how many lines of code did that take you, looping through inner exceptions and concatenating strings versus one call to Exception.ToString()?

  4. While we are having a go at the code, why not point out the string concatenation in the middle of the string.format as well as catching the high level Exception and logging it using the Info method without rethrowing.

Comments are closed.