CODEWITHSUNDEEP
c#.netexception
Silencer
Silencer

Reputation: 1618

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.

Upvotes: 5

Views: 2507

Answers (3)

Ehssan
Ehssan

Reputation: 759

Since you seem to use exactly the same behavior in every one of your methods, I strongly recommend using an aspect-oriented programming framework.

That way you can define an aspect (attribute), which contains your custom exception handling logic, and avoid having the same boilerplate code in every method.

Your class might look like this if you use aspects:

public class PieRepository
{
    [LogExceptions]
    public void AddCherryPie(string incredientA)
    {
        ...
    }
}

And your aspect could look like this:

public class LogExceptionsAttribute : OnExceptionAspect
{
    public override void OnException(MethodExecutionArgs args)
    {
        // Create a log message, you can access the method info and parameters
    }
}

If you attribute your methods to use aspects, the aspects will be called every time the methods execute, so make sure to use a framework that supports compile-time weaving (as opposed to runtime weaving, which has a huge impact on your application's performance).

See http://doc.postsharp.net/exception-handling for an in-depth example.

Upvotes: 2

JuanR
JuanR

Reputation: 7783

A couple of ways you can go about this. The simplest is to use the Data property of the Exception class:

public void AddApplePie(string incredientA, string incredientB)
{
    try
    {
        ...
    }
    catch(Exception ex)
    {
        ex.Data["IncredientA"] = incredientA;
        ex.Data.Add("IncredientB", incredientB);
        throw;
    }
}

Alternatively, you could create a custom exception that contains additional information, then throw that one with the original exception as the inner exception.

To give you an idea, consider the following:

public class PieRepositoryException : Exception
{   
    public PieRepositoryException(string message, Exception innerException, params string[] ingredients):base(message, innerException)
    {
        Ingredients = ingredients;
    }

    public property string[] Ingredients { get; private set; }
}

You can then do this:

public void AddApplePie(string incredientA, string incredientB)
{
    try
    {
        ...
    }
    catch(Exception ex)
    {
        throw new PieRepositoryException("Error adding apple pie.", ex, incredientA, incredientB); 
    }
}

Your higher level code can then implement a strategy to either catch particular exceptions, catch generic ones and output all their properties to a log or use formatters to output to the log based on the type.

Upvotes: 0

Peter Bons
Peter Bons

Reputation: 29780

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.

Upvotes: 3

Related Questions