Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am catching exceptions to provide better information for the logs to make debugging easier in production. I was wondering if this code follows best practices as 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 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;
    }
    catch (InvalidOperationException ex) {
        _logger.Fatal("Razor parse failed: trying to compile layout on it's own", ex);
        throw;
    }
    catch (Exception ex) {
        _logger.Fatal("Razor parse failed", ex);
        throw;
    }
    return content;
}
share|improve this question
3  
Yes this is correct, only you can restrict logging by not saying throw, raise some other exception with inner exception message –  paritosh Aug 1 at 10:06

3 Answers 3

up vote 3 down vote accepted

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.

share|improve this answer
    
Your approach seems the cleanest and will make the code more readable. I'm going to remove the try/catch from the parse method, and only catch higher up if there's something to handle. Global handlers seem like a good idea. No one has commented yet, but could you see any drawbacks from doing this ? Cheers –  ThunderDev Aug 5 at 7:39
    
This is good design. It is not always possible to follow it though. For example, if you are developing some framework or class library, you might want to do both: log critical exceptions to some local log file (so you can ask for it when angry mob starts throwing rocks at you) and rethrow exception for external code to handle. If you are developing a stand alone application, then you should follow Vince's advice and catch exceptions you can't handle straight away on the highest possible level. –  Nikita Brizhak Aug 5 at 8:18

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.

share|improve this answer
    
Thanks, good point –  ThunderDev Aug 5 at 7:31

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.

share|improve this answer
    
Thanks, looks like a good idea. Would it be possible to have a base exception called LoggedException, and have a RazorException that inherits from it, then catch the LoggedException as shown in your example and be able to determine it's subtype (RazorException here) so that I can handle it the way I want ? Hope I'm being clear –  ThunderDev Aug 1 at 15:28
    
You should be able to create a RazorException that derives from LoggedException (which in turn should derive from Exception). You should make sure the RazorException constructor calls the base constructor that way it will run the logging automatically. In the LoggedException constructor you should check any inner exceptions that are added with an if(!(innerEx is LoggedException)) { /* Do Logging */ }, since this will check if an object is compatible including derived types (so this will also be true for RazorException). –  Ben Aug 1 at 16:03
    
This might be of interest. –  Ben Aug 1 at 16:09

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.