Silencer Silencer - 8 days ago 9
C# Question

Exception logging in all methods

We have repository code that looks something like this:

public class PieRepository
{

public void AddCherryPie(string incredientA)
{
try{
...
}
catch(Exception ex){
log("Error in AddCherryPie(" + incredientA + ")");
throw new Exception("Error in AddCherryPie(" + incredientA + ")", ex);
}
}

public void AddApplePie(string incredientA, string incredientB)
{
try{
...
}
catch(Exception ex){
log("Error in AddApplePie(" + incredientA + "," + incerdientB + ")");
throw new Exception("Error in AddApplePie(" + incredientA + "," + incredientB ")", ex);
}
}
}


So this
try -> catch -> log -> throw new
is present in most of repository methods and other important methods in the project.

We had an argument about this today, as I have never seen someone suggest such type of error handling, but the main argument is that we and support need to know exactly what happened, and any other type of exception handling wouldn't give us exactly this...
Could someone tell if this is ok?

Edit: Added original exception message when throwing error.

Answer

Never create a new exception., rethrow it using just throw or at least include the original exception as the inner exception. Otherwise the stacktrace is further up the chain is not correct anymore. It will originate where you did throw new Exception.

Better is:

public class PieRepository
{
    public void AddCherryPie(string incredientA)
    {
        try{
            ...
        }
        catch(Exception ex){
            log("Error in AddCherryPie(" + incredientA + ")");
            throw
        }
    }

or

    public void AddApplePie(string incredientA, string incredientB)
    {
        try{
            ...
        }
        catch(Exception ex){
            log("Error in AddApplePie(" + incredientA + "," + incerdientB + ")");
            throw new Exception("Error in AddApplePie(" + incredientA + "," + incredientB ")", ex); // add original exception as inner exception!
        }
    }
}

Personally, I would Really recommend to just use throw instead of throw new Exception("...", originalException). By always throwing away the original exception you cannot decide what to do later on in the flow. What error to present the user? Action could be different for a database error or validation error (like giving message "database unavailable" or "ingredient A not found") than for a programming error.

The overall method is valid. It is good practice to log it early since you then know the arguments of the method and context of the error. By rethrowing you can handle the error in the UI again, by presenting a message to the user or take other actions depending on the type of the exception.

For some thoughts about error message read this: http://blog.ploeh.dk/2014/12/23/exception-messages-are-for-programmers/

Now, if you really want to do this for every method use Aspect Oriented Programming (AOP, See https://en.wikipedia.org/wiki/Aspect-oriented_programming). With that kind of technique you can do it using for example an attribute. Use PostSharp for example: http://doc.postsharp.net/exception-handling. Logging shouldn't clutter the code.