Handling exceptions for providing log information while debugging

Question

I am catching exceptions to provide better information for the logs to make debugging easier in production. Does this code follow best practices, since I'm not really handling the error but just logging extra information? I was also considering creating a custom RazorException, and throwing that instead. What do you think?

public string ParseFile(string templatePath, object model = null)
{
    //some code removed for clarity

    try
    {
        ITemplate template = _templateService.Resolve(templatePath, model);
        ExecuteContext context = new ExecuteContext();
        content = template.Run(context);
    }
    catch (TemplateCompilationException tcex) {
        _logger.Fatal("Razor parse failed: check for syntax error", tcex);
        throw;
    }
    catch (InvalidOperationException ex) {
        _logger.Fatal("Razor parse failed: trying to compile layout on its own", ex);
        throw;
    }
    catch (Exception ex) {
        _logger.Fatal("Razor parse failed", ex);
        throw;
    }
    return content;
}

Answer 1

I would avoid those throw statements entirely.

You should only be catching exceptions if you plan on doing something with those exceptions. What you're doing is just logging and redefining the exception message.

IMO a better approach is to put your logging code in the global exception handler. You'll have the complete stack trace, Exception type and if you're deploying PDB's with your production code (I hope you do), then you'll have line numbers as well.

Search the web for these global handlers:

protected void Application_Error(object sender, EventArgs e) in Global.asax

AppDomain.CurrentDomain.UnhandledException

TaskScheduler.UnobservedTaskException

Update

Following up from one of the comments from @nikita-brizhak, if you're developing a library then you might want to log those exceptions depending on your use case.

Another case I can think of is wrapper type classes. For example if your application connects to a MySQL and an Oracle database, you might have an abstraction over those connection methods which catch MySqlConnectionException and OracleConnectionException and rethrow a generic DbConnectionException with generic information.

Answer 2

Exceptions have Message property, which you should use, if you want to tell someone what went wrong in human language. You can then use String.Format("Razor parse failed: {0}", ex.Message) for all your exceptions. This will allow you, for example, to throw multiple InvalidOperationExceptions in the future without worrying about how you are going to log those.

Not to mention, that the guy debugging your code will likely prefer to know what went wrong straight away, without the need to look for log messages in your code or in log files.

Answer 3

As I understand it, paritosh suggests a good way to avoid multiple information being dumped into your log by re-throw the error as an new type that won't be used in your logging mechanism. For example:

public string ParseFile(string templatePath, object model = null)
{
    //some code was removed for clarity

    try
    {
        ITemplate template = _templateService.Resolve(templatePath, model);
        ExecuteContext context = new ExecuteContext();
        content = template.Run(context);
    }
    catch (TemplateCompilationException tcex) {
        _logger.Fatal("Razor parse failed: check for synthax error", tcex);
        throw new LoggedException(ex);
    }
    catch (InvalidOperationException ex) {
        _logger.Fatal("Razor parse failed: trying to compile layout on it's own", ex);
        throw new LoggedException(ex);
    }
    catch (Exception ex) {
        _logger.Fatal("Razor parse failed", ex);
        throw new LoggedException(ex);
    }
    return content;
}

Now when you have nested try catches, you won't get many log messages that are all referring to the same issue. For example you can now add error handling outside of a call to the ParseFile method:

string result;
try
{
    result = ParseFile("myTemplatePath", null);
}
catch (LoggedException ex) {
    // this has been logged already
    throw;
catch (Exception ex) {
    _logger.Fatal("Razor parse failed", ex);
    throw new LoggedException(ex);
}

It may even be worth investigating a design that makes the logging part of the LoggedException constructor. So you could have this:

catch (InvalidOperationException ex) {
    throw new LoggedException("Razor parse failed: trying to compile layout on it's own", ex);
}

The constructor could also do the check if the inner exception that is being passed to it is also a LoggedException then it doesn't need to log it. This would remove the need to keep checking for LoggedException in the code.