Scott Hanselman

Hungarian notation. (Of course it's an object.) ;)

Whole concept is wrong. What kind of "HandleException" accepts string? String cannot contain enough information about exception.

Other than all the obvious stuff like the swallowing then re throwing different exception type weird stuff going on, i don't like comparing bool values to true. Looks amateur.

Properties.Settings.Default.TraceLogging == true

* losing all stack info by eating the original exception and trowing a new one

It is losing the true stack trace by adding an additional layer.

Swallowing all exceptions that might be thrown when trying to log the error message
Not checking strMessage for null
Throwing a non-specific Exception object
Prepending a new line to the message when throwing the exception? Why do this?

The whole thing looks kind of backward though. Why would you throw a general exception containing only a string as information?

Here's the HandleException code that I run on all of my web sites.

Yes, I often get mailbombed but if I ruin someones day when they try to use my sites I think I deserve it and my error rate is usually down to about 1 per 500k hits.

		public static void HandleException(Exception exception, HttpContext context)
		{
			HttpException httpException = exception as HttpException;
            if (httpException != null && (httpException.GetHttpCode() == 405 || httpException.GetHttpCode() == 404 || httpException.GetHttpCode() == 403))
			{
				context.Response.StatusCode = httpException.GetHttpCode();
				context.Response.SuppressContent = true;
				context.Response.End();
				return;
			}

			SmtpClient mailClient = new SmtpClient(System.Configuration.ConfigurationManager.AppSettings["MailServer"]);

			StringBuilder errorMessage = new StringBuilder();

			if (exception is HttpUnhandledException)
				exception = exception.InnerException;

			errorMessage.Append(exception.ToString());

			if (httpException != null)
				errorMessage.Append("\n\nHTTP EXCEPTION CODE: " + httpException.GetHttpCode());

			if (exception.InnerException != null)
			{
				errorMessage.Append("\n\n ***INNER EXCEPTION*** \n");
				errorMessage.Append(exception.InnerException.ToString());
			}

			if (context != null)
			{
                //if (context.Request.IsLocal)
                //    return;
                errorMessage.AppendFormat("\n\nRequest Path = {0}\n", context.Request.Url);

				errorMessage.Append("\n\n ***REQUEST PARAMETERS*** \n");
				foreach (string LName in context.Request.Params.Keys)
				{
					errorMessage.AppendFormat("\n{0} = {1};", LName, context.Request[LName]);
				}

                if (context.Session != null)
                {
                    errorMessage.Append("\n\n ***SESSION VARIABLES*** \n");
                    foreach (string key in context.Session.Keys)
                    {
                        errorMessage.AppendFormat("\n{0} = {1};", key, context.Session[key]);
                    }
                }
			}

            System.Diagnostics.Debug.Print(errorMessage.ToString());            

            try
			{
				mailClient.Send
				(
					SiteName + " Web Server <server@site.com>",
					System.Configuration.ConfigurationManager.AppSettings["ErrorEmail"],
					SiteName + " Error " + Guid.NewGuid().ToString(),
					errorMessage.ToString()
				);
			}
			catch (System.Net.Mail.SmtpException)
			{
			}
		}

Bryan - Why not let your Exceptions bubble up higher and simply use ELMAH? It can store more info than you are saving, it can email you, or just make an RSS feed of the exceptions.

The biggest for me is losing the inner exception but throwing a new one instead of wrapping the original exception after logging it. This makes me sad.

Well...that's a hell of a smelly code, eh?

The things I noticed:

Hungarian notation

== true is meaningless and compromises readability

Flush and Close should be inside a finally block

Throwing a new exception without passing the original exception leads to an inaccurate stack trace, making it harder to see what's wrong

Everything! Dont know where to even begin so will quite!!

Well, if the If clause evaluates to false (not to forget the stuff inside can do other bad stuff), then the function it's just another "throwerer of exceptions"- it doesn't actually handle them well if it's throwing a new one as a return.

In addition to the other comments about losing exception information, the TraceSource usage looks pretty dodgy:

1. It's expensive to set up. 2. Flushing it every time isn't great unless you intend to let the program exit. 3. It's externally configurable - you don't have to set up your own internal configuration (that Settings lookup)

Plus, the function is misnamed. It doesn't actually handle the exception...

LOL @ the ToUpper(), I bet that's a fun trace to read. :) It would be even better if it prepended "ZOMG DISASTER!!!!!!1" to every message.

-Shouldn't catch exceptions that we can't do anything about them. It hides critical errors
-and what if tracing is not enabled it is just swallowing exception and raise new exception with less information and if developer didn't handle that again we may see exception screen

1. Exception stack will reset which makes debugging the source of the exception harder, impossible
2. The above could introduce performance issues in a production scenario when this happens a few hundred times per second
3. To continue the exception stack should just Throw instead of throw new Exception(Environment.NewLine + strMessage);
4. TraceEventType.Information should use the Error or Warning
5. strMessage.ToUpper() Oh god!

I could go on...

After taking another look, the first line is enough ammo to crucify. HandleException taking a String in...

if (strMessage=="true")
catch {
new Sarcasm();
}

- Why is not using a TraceSwitch?
- Agree with others, == true is not appropiate.
- Agree with others about the finally block.
- the input parameter has to be an exception not a string, at the end just re throw the exception or throw a custom exception setting the received exception as the inner exception of the new one.

* public void HandleException should be static

* a string is not an exception. It should be renamed to Handle[Message|Trace] or it should accept System.Exception

* it should accept an Exception object.

* if (...) block should be revsered for easier reading; i.e.

if (!Properties.Settings.Default.TraceLogging [== true]) { return; }

* TraceEventType should be Exception, or equivelent with reference to second point above

* Don't do ToUpper(). readability context is lost.

* Should write stack trace to log

* objTrace.Close() tends to point to IDisposable implementations, so the TraceSource object should be something like:

using(TraceSource objTrace = new TraceSource("YadaYadaTraceSource"))
{
objTrace.TraceEvent(TraceEventType.Information, 5, strMessage.ToUpper());
objTrace.Flush();
}

* Exception shouldn't be rethrown as the Trace code should be in the application's Application_Error event

* If it's not in Application_Error... throw new Exception(strMessage) destroys call stack. Should be:
throw [exceptionParam]; // still borks call stack
Or
throw new Exception(exceptionParam);

* Should use a better typed Exception when calling throw new... something like MyCustomTracedException

Minor disagreement on the == true comments. While I agree that

   if (Properties.Settings.Default.TraceLoggingEnabled == true)

would be superfluous, if I am reading code and see

   if (Properties.Settings.Default.TraceLogging)

I instinctively want to check the type of the TraceLogging Property

This code can't even throw an exception. The try-catch is superfluous. (Except maybe the string who could be null but just add a check for that).

I must admit to having used a block of code like that one or two times (let's be honest here)

When?

Usally it's the "Last Chance - OMG, all my other trapping didn't work, try the last chance log, and the result of coming out of there with an error is usually a program termination (with a graceful shutdown routine), or a "reset the program back to initial startup config". It's NOT used on web servers, but on Winforms - and the graceful shutdown is to let the user know "You are about to lose the program, call the developers, as something is DEEPLY wrong"

In addition, it's usually in places where you come in, and find the dev group has NO error handling at all, no logging, and when things crash, you HOPE the end users remember something - it's the "OMG, let's add this in, so we have SOMETHING while we refactor/clean up" - problem is, they have a tendency to stick around, because places with problems that bad tend to take a LONG time to refactor their code (Why is it that I'm the ONLY person in my current group with a refactoring tool, and with unit tests? Why can't I convince folks thses aren't optional? - nevermind)

Let's see:
1. Should this code be part of Utils.cs? I know developers like to have a place to stuff all the code snippets that they don't know where they should belong.
-> Try to see if a Logger class makes more sense. There are plenty of 'common' logging interface libraries online that allow you to later switch your logging implementation later on.
2. As others point out, the name HandleException does not make sense. The function does not 'handle' the exception, it merely 'log' the message.
-> Change it to TraceLogMessage, LogMessage, or LogException
3. Again, others has pointed out The parameter is string. If it is for logging message, it is fine.
-> Optional improvement -> adding more context to the message
4. strMessage?
-> Rename it to only message
5. Make the if statement more readable
--> something like > if (TraceLoggingEnabled) { ... }
6. the if statement is a guarded statement
--> the whole statement should be be >> if (!TraceLoggingEnables) throw ....
7. throw new Exception(Environment.NewLine + strMessage) is misleading
--> should be throw new Exception("Trace logging is not setup correctly, .... blahblahblah ...");
8. Don't swallow exception. If this is the last resort of handling exception and you don't want to crash user's app, you should trust the base app (ASP.NET or Windows app) will have a global default exception handling process that is fully tested
9. Hard-coded TraceEvent() parameters
10. I am not familiar with TraceSource, but shouldn't we have TraceListener set up here?

Here is one possible rewrite:

... in TraceLogger.cs ...

...
class TraceLogger {

private TraceSource trace;

...
private void LogMessage(String message)
{
if (!TraceLoggingEnabled) throw new Exception("Trace logging is not setup correctly, .... blahblahblah ...");

trace.TraceEvent( ... );
trace.Flush();
trace.Close();
}

- Method name is misleading, instead of handling exception, it's actually throwing one every single time.
- Why a new line is being added before the message of the exception that's thrown at the end.
- Capitalizing the message when calling TraceSource.TraceEvent is a bad idea.

Of course, all the comments before this one are making some good points:
- A string is not an exception.
- Hungarian notation.
- Better use "using" for the TraceSource object.
- Don't use "== ture".
- Didn't check if strMessage is null.
- Throwing a non-specific Exception object
- Losing all stack info by eating the original exception and trowing a new one

MY NAME IS CHRISTY NICHOLE I HAVEN A DELL COMPTERS HAVE A PASSWORDS ON AT

This code throws as much exceptions as it handles.
Who's gonna catch those, this method again?
notation and other cosmetics aside

It's misnamed. I'd call that something like LogExceptionToTrace.

It should be accepting an exception as a parameter, not a string.

It should re-thrown the original exception, not a new one. If they wanted to record in the stack trace that the exception passed through this method, they could throw a new exception with the original as the inner exception and some appropriate text as the message.

It's a bad idea to just eat exceptions. At the very least, if the attempt to log to the trace caused an exception for some reason, I'd throw a new exception of some form, possibly a custom exception that inherits from ApplicationException if there's no appropriate specific exception in the framework. Perhaps a TraceLoggerException?

@Sean Stapleton - in regards to your comment...

"if I am reading code and see

if (Properties.Settings.Default.TraceLogging)

I instinctively want to check the type of the TraceLogging Property"

... there is absolutely no reason to check the type of the TraceLogging property. If the code compiles, the type is a boolean. However, for readability, the property should be named "IsTraceLoggingEnabled".

Cheers,

G

I may be naive but
1. Why log an exception as Information? should this be warning, error or some thing else...
2. Why capitalize the message/exception string?

TraceSource objTrace = new TraceSource("YadaYadaTraceSource");
objTrace.TraceEvent(TraceEventType.Information, 5, strMessage.ToUpper());

The person who wrote the code must be a Pro.

I mean pro in writing these such code colon p.

.ToUpper(); is great idea. I'll use it in future. :P

In VB this would be much simpler and cleaner to write.

On error resume next

Beat that.

No point reciting the same issues everyone else has stated - however I did write this blog post a few weeks back about using AOP. I'd be interested in (constructive) criticisms

http://tapmantwo.blogspot.com/2011/01/implementing-rudimentary-exception.html

Cheers :)

@chrissie1 LOL!!!!

This function should simply not exist. I thought we had passed that stage. Don't touch those exceptions, plug ELMAH or SmartAssembly or the likes into your app and be done with it already.

I haven't used ELMAH that heavy yet, but is ELMAH able to log exception even if they are handled? Seems that ELMAH only logs errors that lead to yellow screens of death, i. e. unhandled exceptions?!

@Timm Krause

You CAN use ELMAH to log handled errors, but you'll need to ask it nicely :D
It's done using the ErrorSignal class. Take a look at this tutorial, the example is near the end:

http://www.asp.net/hosting/tutorials/logging-error-details-with-elmah-cs

Why are you handling an exception then rethrowing one, seems counter-intuitive to "Handle" a message then not handle it by throwing it back up. Why would you even want a subscriber of your web site to see an error, wait you said app config so this is some type of desktop app/service but why would you want any consumer to see an error. Rename it to LogException and write a real Handle Exception method or better yet ELMAH or EntLib the exception and stop trying to re-create the wheel.

* If the purpose of this code is to just log "exceptional" exceptions for further evaluation, then this method should take in an Exception object instead of a string to get the full exception information including call stack. What good is it going to be to have a log somewhere full of "Object reference not set to an instance of an object", with no other information to help troubleshoot?

* We can't see the actual TraceListener here, but if the original Exception was due to a network error, then logging to something like a DatabaseTraceListener will also fail, so nothing will ever get logged and no one will ever be able to tell there was a problem. This should fall back to logging to a text file/windows event log/or an in-memory log (like the in-memory option for ELMAH).

* If the tracelistener logs to a console or text file, then it'll be harder to check up on these logs in a load-balanced, multi-server environment (the logs will only be local to that server/box).

* If this is just logging code, then why even bother to write this code yourself? Just use a tried-and-true system like ELMAH. Do you really think that your homegrown exception handling utility method is going to be better or more powerful than a system that has been is use for years with lots of developer feedback? Seems like a waste of development time.

[:: nods in agreement at current comments ::]

I'm also wondering about the placement of this code. The Global class (global.asax for web or global.asa for desktop) have a Application_Error method that I think would function better for this high level type of error catching. Isn't the general idea that you should only try/catch errors you can genuinely handle within your individual methods and let the global handler work with the rest of things?

As others have stated, using a logging framework of some type, would be an easier way to handle this.

Also, I'm not sure; it's been awhile since I've used a TraceSource, but if I remember correctly, the whole testing if trace is enabled could be omitted. You can setup trace in the in the web/app config files, and if disabled (enabled by default console apps, not sure about web apps), I believe that they will just be skipped during the program run. You can also enable and disable on individual trace sources using source switches in the config file.

All in all, the whole method just looks "dirty", like others have stated.

Not that the original code as written could let you know, since all it passes is a string, but if you're handling an actual Exception object, you should check that it's not an OutOfMemoryException. In that case it's not even safe to new up an object. Usually best just to let the OOM exceptions percolate and hope there's a global error handler that can log it without needing to allocate.

Line 4 is wrong and a line 3.5 is required. It should be

//Log to trace file only if app settings are true
// To be sure, to be sure, to be sure
if (((Properties.Settings.Default.TraceLogging == true) == true) == true)

They didn't follow Rule #6: Create a global error handler that logs everything.

Magic number.

objTrace.TraceEvent(TraceEventType.Information, 5,.... 

What is 5?

Lines:

* 1. Rename the Method. Why is the message just a string? Parameter should not be string. Parameter name should not have str
* 3. Comment is not required based on the next line being readable.
* 4. Possible null exceptions? == true is not required (but not a showstopper!).
* 8. Could use var. Poor variable name. TraceName should be a constant/readonly property.
* 9. Why is the event type info, shouldn't it be an error? What is 5? Why is the message upper? (We hope its not a "business requirement!")
*10. Should this be done with a 'using' block?
*13. Should we only catch exceptions we want?
*15. It might be better to let the trace error bubble up
*18. Shouldn't be using the generic Exception. Why to we add a new line?

Strategy

* Worth running FxCop/ReSharper
* Hard to tell how wrong it is without more context.
* How testable is this? Should the TraceSource be created via IoC/Injection?
* Should the Trace section be pushed into a different method/class? (SRP)
* Should Elmah be used?

Although the == true is bad, I think it’s more important to look at the testability and S.O.L.I.D. design.

A couple of people have pointed out that throwing the base class "Exception" isn't the best plan, but not until mikeblake just above my comment has anybody (that I've seen), hinted at not *catching* the Exception base class. Mike - it's quite fitting that you mention FxCop, because it has an analytics rule that you shouldn't catch the Exception base class.

I noticed not a single person mentioned Microsoft's own HealthMonitoring provider. We use it on all our web sites, requires little to no additional code or logic, and allows IT staff to tap into it via Event Logs, WMI, email alerts or SQL logging. Why people continue to grow their own I'll never understand.

Shame on you Scott for not promoting it more (vs ELMAH).

@Norville Rogers

I'd respectfully disagree. We've used HealthMonitoring, rolled our own, and elmah. We finally settled on Elmah as it just works, is dead simple to configure for our needs, and just seems a lot less cumbersome than HealthMonitoring, although HealthMonitoring can do a few things that elmah can't.

Here is how I do it in Squiggle messenger but the reason I do its because there is nothing I can do about connectivity problems in a WCF method call besides logging it and trying again or if an exception occurs in a fire and forget kind of scenario where the completion of operation isn't all that important.

        public static T EatTheException<T>(Func<T> action, string actionDescription, out bool success, out Exception ex)
        {
            ex = null;
            try
            {                
                T result = action();
                success = true;
                return result;
            }
            catch (Exception exception)
            {
                ex = exception;
                Trace.WriteLine("Erorr occured while " + actionDescription + ": " + ex.Message);
            }
            success = false;
            return default(T);
        }

Yikes, I'm a little late jumping in here after so many great comments. This is why I like you folks so darn much...you're active. Thanks for all the great comments! Here's a few of mine before I update the post.

@viscious - Yes, while Microsoft has had HealthMonitoring in ASP.NET for almost 6 years, I'd agree that ELMAH is just darned simple and joy to use. They are both useful...I wonder if someone should make a HealthMonitoring NuGet package?

@Norville - I think that "shame on you" is a little much. I prefer Elmah. Why do you like HealthMonitoring more?

Here's my thoughts about the code and the idea behind it (which are two separate things, of course), in no order:

* You should really only "handle (catch)" exceptions if you can do something about the root cause. Logging isn't the same has handling. 
* Don't catch everything, catch specifics you can deal with. Catch DataException or ReadOnlyException, never ApplicationException or just Exception. Be specific, let the rest fly
* Don't catch something specific and throw something new and vague. Worst case, rethrow with "throw"
* Methods that take parameters like string message usually could be taking something with a lot more information and context
* Catching an exception and not rethrowing will cause you to lose the real stack trace. Again, bad idea to toss good info.
* Don't call ToUpper unless you want a culture-specific uppercase implementation. Use ToUpperInvariant. See the Turkish "i" problem.
* Avoid creating half-done loggers, use NLog or Log4Net. Make sure your application takes advantage of the great and trusted open source libraries out there like these loggers and/or ELMAH. If you have to you can send your exceptions *to* ELMAH:  ErrorSignal.FromCurrentContext().Raise(e);
* Remember that Application_Error exists. Catch exceptions as high as you can, not as low.

Scott if we catch exceptions too high, we loose context and cannot take corrective action if possible. I think making it a general rule isn't right. Catching exceptions very high is only suitable when you're not going to do anything about it besides eating it, logging it or giving user the yellow screen.

In the apps we use at work, we use a common logging implementation, usually log4net. Application-level exceptions at the global level get logged, and with log4net's "GetCurrentLoggerForClass" we can log specific things where we need to, which provides the detailed context we need (x happened in y class in z method).

With DI or even the regular log4net implementation, you can log anywhere in your app. We try to do everything possible to fallback on an acceptable value during an exception, then silently log it for our benefit. We also capture (for web apps) the request details like who was logged on, query strings, machine name, etc.

I also think exception handling should really only occur at the caller level; i.e. in your Library/Business logic/etc. you shouldn't be catching errors (maybe logging details, though). In your UI-layer (Controllers, whatever) you'd handle any exceptions. If I have some data access code, I don't want it swallowing the exception, I want it to bubble up to my UI where I can display a proper message and log it if I need to. Some utility functions like user lookup (especially AD calls) we've used fallback values before in those functions.

Before I've implemented a wrapper routine that takes an Action and invokes it within a try...catch. So you'd do:

base.TryExecute(() => {
    var repo = MyRepository.Get(something);

    repo.SaveChanges(); // could throw an exception
});

The function wraps the call and logs it/handles it behind-the-scenes. TryExecute accepts some overloads for messages, ignoring errors, etc. It also handled some special errors in a common way (UI exceptions would get shown to the user, for example).

Not saying this is a good way to do it, but just saying I've done it before for our Web Forms applications that kept duplicating error logging and exception handling code all over the place. Previous developers had also used the [bad] approach of communicating via exceptions... which made the issue worse.

I wrote two classic articles on this years ago:

http://gen5.info/q/2008/07/31/stop-catching-exceptions/
http://gen5.info/q/2008/08/27/what-do-you-do-when-youve-caught-an-exception/

Let’s start counting
On Philosophical Level
1) It is trying to handle all exception, even like OutOfMemoryException
2) It is violating fail fast philosophy
3) This might never get chance to execute
On Code level
1) Method signature indicate that original exception details will not be logged
2) If it is framework library, I cannot enhance it due to its signature
3) Line 4 assumes that settings (element/attribute) will be there, in absence of it, it will throw exception and will be not handled it.
4) Line 8 to 11, might not execute (depending upon what exception it is trying to handle) or throw another exception. Every handled exception is creating TraceSource and closing it, this might hamper performance (depending upon Trace listener)
5) Line 13, catch is trying to catch everything (ex. non managed exception)
6) Line 18, throwing new exception is bad idea, this new instance of exception lack details, losing original exceptions stack trace etc.
7) Line 18 might throw another exception in case of Out Of Memory 

I mostly agree with the above, but I think some of you might be missing the intention of the developer. I think what he/she really tries to accomplish is a "power" way of throwing exceptions - namely by getting some logging in there as well. This is probably bad, but if this is the case some of your comments don't really apply. For instance, we're not getting rid of exception information we already have since there is no exception from the beginning.

I imagine the intented code was something like this:

bool success = DoSomeThings(myArg);
if(!success)
    HandleException("Do some things failed for myArg=" + myArg");

Rather the function should probably be called something like ThrowAndLog(string msg). If you don't like the Exception type you could even make it generic with ThrowAndLog<T>(string msg) where T : Exception. But is this a good idea? Probably not, since we're relying on the thrower to log and not the catcher which seems like a strange separation of concerns. And some other things that have already been mentioned such as performance, catching edge case exceptions, stack trace messing (it's probably not that bad though) etc. also apply

Furthermore, obviously, a lot of other small things apply that you have already spotted.

I misread this as "Good Expectation Management ..." and thought, "Man, we definitely need some of THAT in software development!"

This code doesn't contain enough cowbell!

I tend to think that having a global "handle exception" method is a disingenuous antipattern (forgiving, for the moment, that the method neither handles exceptions (wrong verb, it's logging it) nor could do so with the parameters (string is not Exception)).

If you have, say, a desktop application, then it probably has one UI thread and many non-UI threads. Strategies for handling an exception from a call to DrawLine() on a background thread which is preparing a chart are going to be completely different than handling an exception from a call to DrawLine() in a Button on the UI thread. In the latter, it may be perfectly fine to flush the exception and keep on running -- the button will look a little funny but it's preferable to crashing. However, in the latter case it may be considered data loss. And another thread may be busy doing something that requires allocating memory and the strategy there must also be different.

If you know you can handle the exception, then do so. If you have extra information, then rethrow a new Exception and include the original as the innerException. If you don't know what to do then let the global unhandled exception handler tear down your app and hopefully send you a crash dump or log or something.

You are all, beginning with Scott Hanselman of course, WRONG.

This code is good enough for production use. With one condition :

It must be saved in a file Util.cs ...









and this file deleted, shredded, the whole effing drive should be physically destroyed after!!! =)))))))

Scott, are you still planning to recap the comments and write a follow up post?