CODEWITHSUNDEEP
c#exceptionloggingloadtry-catch
Sam
Sam

Reputation: 25

Catching all exceptions for detailed logging purposes - pros and cons

I'm in the middle of writing a saving and loading system that saves game objects to a JSON compatible format and loads them back. Saving objects is fine and fairly fool-proof. However, I'm in the process of trying to make the loading process more bullet-proof and easier for me to pin-point errors. Obviously as I'm loading data from a JSON file, everything is being converted from strings, with the potential for all kinds of user error.

Each objects loading method contains something similar to this ---

public void Load() 
{
    try
    {
        // Deserialize..
        // Potential for all kinds of exceptions, invalid casts, missing info, etc..
        // For eg:

        bool active = data.GetField("active").Value<bool>();

        // GetField() throws an exception if the field doesn't exist.
        // Value<T>() throws an exception if it's an invalid cast, etc.
    }
    catch(Exception e)
    {
        // ..and those exceptions get caught and logged here.
        Logger.LogError(e.ToString, this);
    }
}

One of the benefits of catching all the exceptions in the objects Load method is that when 'LogError()' is called with the object as a parameter like shown, it will highlight the offending object in the IDE hierarchy that threw the error (whichever one it may be) making it very quick to find the object with the error and debug it's script or saved data.

Is this an appropriate usage of throwing and catching exceptions, or is there a better design pattern I should be implementing?

Thanks for your time, Sam

Upvotes: 2

Views: 440

Answers (2)

Eugene Podskal
Eugene Podskal

Reputation: 10401

There are always a lot of different but close opinions about catching the exceptions, but for your task the given approach is simple and viable. The only thing I'd recommend for you to do is to allow the exception to bubble up:

catch(Exception e)
{
    // ..and those exceptions get caught and logged here.
    Logger.LogError(e.ToString, this);
    throw;
}

Logging is a good idea, but what should your program do if the loading fails? Just log it out and continue? It can be the case, but this load method looks like important and integral part of your program. It does not look like some background work-that-may-fail-or-work-I-don't-care. Well, its your program and I don't know the requirements or context - just an opinion of mine.

The main principle you should base your exception handling on is: to catch exception only when and where you can do something with them (retry, get some relevant current data in the method...), or otherwise catch them as high in the call hierarchy as possible (in your main loop or global exception handler - the higher the better, it means less exception handling for you to duplicate). And if you can't do anything with exception that would guarantee the consistency of your program state - don't catch it.

Ideally all classes should be resilient to failure - if they fail, they either fail and don't do anything afterwards or restore their state to normal. And the most ideal solution is if they rolled back any action's side effects in case of exceptions - transaction-like behaviour. But in real life it is a difficult goal to achieve.

Also, if you don't like similar try-catch constructs for logging everywhere in your code (they are important , but they aren't doing any real job - their presence isn't necessary for the understanding of the main workflow), you may try some of the Aspect Oriented Programming technologies.

Upvotes: 1

Joshua I
Joshua I

Reputation: 560

Your question still missing many basic things like what kind of object "data" is, is it a string , class or any other type.

Well catching exception is never good for performance as long as your code handle all possible exceptions that might occur. There can still be a scenario where exception is thrown.

I would suggest to check for field instead of directly calling field with that name, e,g,

bool active = data.HasField("active") ? data.GetField("active").Value<bool>() : false;

Also, In catch as common scenario better to pass exception object rather than e.Tostring()

something like : 

Logger.LogError(e, this);

To conclude it really depends on application to applications or rather requirement to requirements. In web Application, I usually create HttpModule for logging exception so that I dont have to write catch block everywhere in my code.

public class ExceptionModule : IHttpModule
    {
        public void Init(HttpApplication context)
        {
            context.Error += new EventHandler(OnError);
        }

        private void OnError(object sender, EventArgs e)
        {
            //write logging exception logic.
        }
}

Upvotes: 0

Related Questions