views:

76

answers:

4
public void EatDinner(string appetizer, string mainCourse, string dessert)
{
   try
   {
      // Code
   }
   catch (Exception ex)
   {
      Logger.Log("Error in EatDinner", ex);
      return;
   }
}

When an exception occurs in a specific method, what should I be logging?

I see a lot of the above in the code I work with. In these cases, I always have to talk to the person who experienced the error to find out what they were doing, step through the code, and try to reproduce the error.

Is there any best practices or ways I can minimize all this extra work? Should I log the parameters in each method like this?

Logger.Log("Params: " + appetizer + "," + mainCourse + "," + dessert, ex);

Is there a better way to log the current environment? If I do it this way, will I need to write out all this stuff for each method I have in my application? Are there any best practices concerning scenarios like this?

+3  A: 

As a general rule, I would say strive to log all the information necessary to reproduce the course of events leading to an error. Note that you don't necessarily need to log everything inside the catch block: you can put (debug) log statements around in your code, inside methods called etc. which enable you to follow what was going on directly before the exception. Also, you should put information into the exception itself about the exact symptom what caused it.

IMHO going through all the code to add log statements everywhere may be overkill - or at the least, not cost effective in a real life project. Instead, focus on the most critical code parts to maximize returns from your efforts. These code parts are typically where the most bugs happen and/or the most modifications are (going to be) done. So in practice whenever you need to touch a piece of code, think about logging, check the log statements already present there (if there is any), check exception handling (if there is any - I routinely see not only code like your example which simply swallows the exception caught, but even empty or autogenerated catch blocks in our legacy code... all of these may leave the application in undefined state, which is a BAD THING), and think about whether what is already there is enough for you to reproduce failures and understand what happened if an error occurs. Then improve it as much as you need to and can with a reasonable effort.

It also helps to discuss this topic with your teammates and try to work out a rough project convention of how to log events, how to handle exceptions etc. This might save you a lot of time otherwise spent on chasing bugs and/or improving code produced by your very coworkers :-(

Péter Török
+3  A: 

That is pretty horrible code. It eats the exception, presumably leaving the application in an undefined state. In general, by all means log the exception (but don't wrap every bit of code in a try block), but then re-throw it.

anon
Note that the re-throw should be done using `throw;` and *not* `throw ex;`, to avoid wiping the call stack.
Richard Ev
+1  A: 

Your logging framework should capture as much of the context for you as it can. At the very least it can easily capture the class that the error occurred in. It should also log the full exception including stacktrace and any inner exceptions.

As I answered in your other question you should use different log levels. Once you do this, if you are using an inversion of control container, it is a fairly simple task to wire up an interceptor that will intercept all method calls and if you're in debugging mode log the call, timestamp and any parameters.

George Mauer
A: 

Here is a example from Rico Mariani (CLR perf )why you shouldn't catch all exceptions. It can be really hard to diagnose the real problem.

Naveen