views:

106

answers:

3

In my current project I see a lot of this type of code:

public User GetUserByName(string userName) 
{
    try
    {
        // Lots of code here to check if the user is in the cache,
        // get it from the DB if not, set properties on it, etc...
    }
    catch (Exception ex)
    {
        throw new Exception("Exception getting user by name, username: " + userName, ex);
    }
}

I appreciate the intent here, having the local variables is really useful for debugging, but putting a try/catch around every method seems like far too much work, and it really doesn't help the readability. For now I'm ignoring the fact that we're throwing a System.Exception, naturally a more specific type would be better but it's not the focus of my question. Also it probably goes without saying that this gets copied and pasted around heavily without any modifications to the exception message...

So my question is: how do you capture this kind of contextual information (e.g. username in the above example) for debugging? Let's assume for the purposes of the question that the method is otherwise well-written and any specific exception checking is taken care of, and all I want is to add context to any exception that can't be handled here and needs to bubble up to the top-level handlers.

My first thoughts:

  1. Lean on the logging framework to trace out each method call with it's parameters, which again is really boilerplate-y and is far more logging than appropriate for production.

  2. Bring in some AOP and intercept every method (or a limited subset), checking for an exception on the return and either logging the parameters or adding them to the thrown exception.

Any other opinions welcome!

+1  A: 

I add a log message with the method name and then rethrow the exception. The exceptions should only be occurring for unforseen conditions and therefore are for the use of the programmer to fix. The most important thing for the programmer is to know where the exception occurred and that's what the log message does (if it's a generic method name I add the class name into the log message as well).

public User GetUserByName(string userName) 
{    
    try    
    {        

    }    
    catch (Exception ex)   
    {        
       logger.Error("Exception GetUserByName. " + ex.Message);
       throw;
    }
}
sipwiz
Exception can be thrown and this doesn't mean you have to fix something in the code. For instance db connection can be lost.
Mykola Golubyev
If you anticipate the exception happening, such as a db connection being lost, then you can handle it appropriately. If you get an exception you weren't expeting then you do need to fix it.
sipwiz
@sipwiz - this is much much better than the rethrow, still, don't you find it repetitive having to wrap all your methods in a try/catch block?
Jon M
I don't wrap all methods in a try/catch block only ones where I judge there is a chance of an unforseen exception happening. I'll always catch exceptions at the "main" level as well so that the app won't crash but in order to track down the exception it's a big headstart to know the method.
sipwiz
A: 

Your Try-Catch blocks should only be around pieces of code that are capable of throwing exceptions (Anything involving user-input, anything involving an external datasource) In those cases it is best to have only the relevant local variables something that will be logged. These exceptions are expected (unavoidable) and so you need to log them in a way that differentiates between expected errors, and code errors.

If you are not adding extra context to the message, then you are misusing your logging tools. It is always better to catch and rethrow rather than not catch at all (it executes the finally blocks) but you should also add a relevant note to every logging message, otherwise you will actually be damaging your ability to debug the code, because you have no idea where the exception was thrown from.

Also, .Log files are cheap, there's no harm in having a LOT of information in them whenever you throw an error. The more error logging you have, the easier it will be to debug a production application.

A: 

Thrown Exception is the normal behavior of the code. There is no reason to dig the log to find out why or who did that. And change your code to make this never happen again.

Thrown Exception means something goes wrong. But not with the code. It can be a problem with the DB connection, user Input, other environment stuff.

The only Exception you can catch for log with the reason is the Logic one. To find out where the programmer made an error. And only this exception you have to fix and log. All other exceptions should be caught ones at the appropriate level of abstraction without re-thrown.

Mykola Golubyev