Nested Try/Catch

Is having a nested Try/Catch a signal that you're not coding cleanly? I wonder because in my catch I'm calling another method and if that fails I get another runtime error so I'm tempted to wrap those calls in the catch with another try/catch again. I wond开发者_运维问答er if this is normal to do this?

e.g.

    catch (Exception ex)
    {
        transaction.VoidOrder(transactionID);

        LogError(ex.ToString());
        Response.Redirect("Checkout", false);
    }

so the VoidOrder or even the LogError methods could bomb out. Right now when I call VoidOrder, I get a null ref on transactionID because it calls a BL method and in that BL method I'm re-throwing so I can catch it at this higher level in code above. But if I'm throwing again inside a catch then I need to catch that as well.

Here's how we approach the problem:

All calls from the UI/codebehind level to other tiers use a try-catch, where we always catch a custom exception. All actions taken by underlying layers have their own try-catch, which log, wrap and throw the custom exception. The UI can then rely on this and look for handled exceptions with friendly error messages.

Codebehind:

protected void btnSubmit_Click(object sender, EventArgs e)
{
    //do something when a button is clicked...
    try
    {
        MyBL.TakeAction()
    }
    catch(MyApplicationCustomException ex)
    {
        //display something to the user, etc.
        ltlErrorPane.Text = ex.Message;

        //or redirect if desired
        if(ex.ErrorType == MyCustomErrorsType.Transactional)
        {
            Response.Redirect("~/Errors/Transaction.aspx");
        }
    }
}

BL:

In the business layer, any operations which may fail use a try-catch, which logs and wraps the issue before throwing it to the UI.

public class MyBL
{
    public static void TakeAction()
    {
        try
        {
            //do something
        }
        catch(SpecificDotNetException ex)
        {
            //log, wrap and throw
            MyExceptionManagement.LogException(ex)
            throw new MyApplicationCustomException(ex, "Some friendly error message", MyCustomErrorsType.Transactional);
        }
        finally
        {
            //clean up...
        }
    }
}

Exception handler:

The actual exception handler has multiple ways to log, including Event log, file log and lastly email if all else fails. We choose to simple return false if the logger can not do any of the expected actions. IMO this is a personal choice though. We figure that the likelyhood of 3 methods failing in succession (event log fails, try file log, fails, try email, fails) is very unlikely. In this case we chose to allow the app to continue. Your other option would be to allow the app to fail completely.

public static class MyExceptionManagement
{
    public static bool LogException(Exception ex)
    {
        try
        {
            //try logging to a log source by priority, 
            //if it fails with all sources, return false as a last resort
            //we choose not to let logging issues interfere with user experience

            //if logging worked
            return true;
        }
        catch(Exception ex)
        {
            //in most cases, using try-catch as a true-false is bad practice
            //but when logging an exception causes an exception itself, we do
            //use this as a well-considered choice.
            return false;
        }
    }
}

Lastly, as a fail-safe, we do implement the Application_Error global event handler (in Global.asax). This is a last resort for cases where we did not properly try-catch something. We generally log and redirect to a friendly error page. If the above custom error handling is done successfully though, very few errors will make it to the global handler.

Hope this might help a little. It's one possible solution. It has worked very well for us for several years on some larger applications..

Nested try/catch blocks are unavoidable in top-level applications that need to log, send messages, or react to exceptions in other non-trivial ways.

There are ways to cut down the number of nested blocks (for example, consolidating error handling using ASP.NET's HttpApplication.Error handler (a.k.a. Application_Error)), but you should catch any exceptions produced by your handling code and implement a backup plan in case all else fails.

One other solution to your nesting issue would be to encapsulate the try/catch logic for the inner functions (LogError, etc) in those functions, instead of depending on the caller to catch them. For LogError, this would make sense because you are probably going to want to handle a broken error logger the same way, no matter who is trying to log an error.

Perhaps is a question whether someone can understand why are you using the nested try's/catches. Sometimes become unavoidable to use them, but I say usually is not pretty.
You should consider that separation of concerns it is a must.

A getControl method should not insert a script into it, neither a save method should do more than save.

Examine what exactly those methods should do and them you will have your answer. It does not looks so awkward to null a order in case of error. Sounds reasonable.

Did you think about using the TransactionScope object instead of trying to roll back your transaction manually? I know that sometimes this is impossible, but most of the times for me the TransactionScope worked a treat. This way, I didn't have to manage roll backs items manually - manual roll backs might be an issue anyway, because the data is kind in a "unstable" state, which might mess up your whole logic.