When so many things can go wrong all you do is try, try, try

Question

Seriously, how can you handle all those exceptions without going nuts? Have I read one too many articles on exception handling or what? I tried refactoring this a couple of times and each time I seem to end up with something even worse. Maybe I should admit exceptions do happen and simply enjoy coding just the happy path? ;) So what's wrong with this code (besides the fact that I was lazy enough just to throw Exception instead of something more specific)? And by all means, don't go easy on me.

public void Export(Database dstDb)
{
    try
    {
        using (DbConnection connection = dstDb.CreateConnection())
        {
            connection.Open();
            DbTransaction transaction = connection.BeginTransaction();
            try
            {
                // Export all data here (insert into dstDb)
                transaction.Commit();
            }
            catch (SqlException sqlex)
            {
                ExceptionHelper.LogException(sqlex);
                try
                {
                    transaction.Rollback();
                }
                catch (Exception rollbackEx)
                {
                    logger.Error("An exception of type " + rollbackEx.GetType() +
                                      " was encountered while attempting to roll back the transaction.");
                }
                throw new Exception("Error exporting message " + Type + " #" + Id + ": [" + sqlex.GetType() + "] " + sqlex.Message, sqlex);
            }
            catch (Exception ex)
            {
                try
                {
                    transaction.Rollback();
                }
                catch (Exception rollbackEx)
                {
                    logger.Error("An exception of type " + rollbackEx.GetType() +
                                      " was encountered while attempting to roll back the transaction.");
                }
                throw new Exception("Error exporting message " + Type + " #" + Id + ": [" + ex.GetType() + "] " + ex.Message, ex);
            }
        }

        try
        {
            Status = MessageStatus.FINISHED;
            srcDb.UpdateDataSet(drHeader.Table.DataSet, HEADERS,
                CreateHeaderInsertCommand(), CreateHeaderUpdateCommand(), null);
        }
        catch (Exception statusEx)
        {
            logger.ErrorException("Failed to change message status to FINISHED: " +
                                    Type + " #" + Id + ": " + statusEx.Message, statusEx);
        }
    }
    catch (Exception importEx)
    {
        try
        {
            Status = MessageStatus.ERROR;
            srcDb.UpdateDataSet(drHeader.Table.DataSet, HEADERS,
                    CreateHeaderInsertCommand(), CreateHeaderUpdateCommand(), null);
        }
        catch (Exception statusEx)
        {
            logger.ErrorException("Failed to change message status to ERROR: " +
                                    Type + " #" + Id + ": " + statusEx.Message, statusEx);
        }
        AddErrorDescription(importEx.Message);
        throw new Exception("Couldn't export message " + Type + " #" + Id + ", exception: " + importEx.Message, importEx);
    }
}

Btw. So many times I tried really hard to be as specific as possible when forming questions - the result was no visits, no answers and no idea how to solve the problem. This time I thought about all the times when someone else's question caught my attention, guess this was the right thing to do :)

Update:

I've tried putting some of the tips into practice and here's what I came up with so far. I decided to change the behavior slightly: when it's not possible to set message status to FINISHED after successful export I consider it as job not fully done and I rollback and throw an exception. If you guys still have some patience left, please let me know if it's any better. Or throw some more criticism at me. Btw. Thanks for all the answers, I analyze every single one of them.

Throwing an instance of System.Exception didn't feel right, so I got rid of that, as suggested, and instead decided to introduce a custom exception. This, by the way, also doesn't seem right - overkill? This appears to be fine with public methods but a bit over-engineered for a private member, yet still I want to know there was a problem with changing message status instead of a problem with database connection or something.

I can see couple of ways of extracting methods here, but all of them seem to mix responsibilities which jgauffin mentioned in his comment: managing database connection, handling database operations, business logic (export data). Say the ChangeStatus method - it's some level of abstraction - you change the status of a message and you're not interested in how this thing happens, how the message is persisted, etc. Maybe I should use a Data Mapper pattern to further separate responsibilities, but in this still quite simple scenario I thought I'd get away with Active Record. Maybe the whole design is so convoluted right now, that I don't see where to make the cuts?

public void Export(Database dstDb)
{
    try
    {
        using (DbConnection connection = dstDb.CreateConnection())
        {
            connection.Open();
            using (DbTransaction transaction = connection.BeginTransaction())
            {
                // Export all data here (insert into dstDb)
                ChangeStatus(MessageStatus.FINISHED);
                transaction.Commit();
            }
        }
    }
    catch (Exception exportEx)
    {
        try
        {
            ChangeStatus(MessageStatus.ERROR);
            AddErrorDescription(exportEx.Message);
        }
        catch (Exception statusEx)
        {
            throw new MessageException("Couldn't export message and set its status to ERROR: " +
                    exportExt.Message + "; " + statusEx.Message, Type, Id, statusEx);
        }
        throw new MessageException("Couldn't export message, exception: " + exportEx.Message, Type, Id, exportEx);
    }
}

private void ChangeStatus(MessageStatus status)
{
    try
    {
        Status = status;
        srcDb.UpdateDataSet(drHeader.Table.DataSet, HEADERS,
            CreateHeaderInsertCommand(), CreateHeaderUpdateCommand(), null);
    }
    catch (Exception statusEx)
    {
        throw new MessageException("Failed to change message status to " + status + ":" + statusEx.Message, statusEx);
    }
}

Answer 1

1. Datasets are the root of all evil ;) Try using a ORM instead.
2. Read about single responsibility principle. your code does a lot of different things.
3. Do not catch exceptions only to rethrow them
4. Use using statement on transaction and connection.
5. No need to catch all different exceptions when all exception handlers do the same thing. The exception details (Exception type and Message property) will provide info.

Answer 2

Exception handling is a well discussed and is implemented in a well varied number of ways. There are some rules I try to abide by when handling exceptions:

1. Never throw System.Exception, generally there are enough types of exceptions to fill your requirement, if not, specialise. See: http://www.fidelitydesign.net/?p=45
2. Only ever throw an exception if the method itself cannot do anything but throw an exception. If a method can recover/handle expected variations of input/behaviour, then don't throw an exception. Throwing exceptions is a resource-intensive process.
3. Never catch an exception just to rethrow it. Catch and re-throw if you need to perform some additional work, such as reporting the exception, or wrapping the exception in another exception (typically I do this for WCF work, or transactional work).

These are just the things I do personally, a lot of developers prefer doing it ways in which they are comfortable....

Answer 3

Additionally to the great answer of jgauffin.

6 . don't catch exceptions just to log them. catch them on the top most level and log all the exceptions there.

Answer 4

Create a log class that handles fall backs for its own failures (i.e. tries SQL, if that fails writes to event log, if that fails, writes to a local log file, etc).

Also I don't recommend you catch an error and throw a different one. Every time you do so you are losing valuable trace/debug information about the original exception source.

public void Export(Database dstDb)
{
  try
  {
    using (DbConnection connection = dstDb.CreateConnection())
    {
        connection.Open();
        using (DbTransaction transaction = connection.BeginTransaction())
        {
            // Export all data here (insert into dstDb)
            ChangeStatus(MessageStatus.FINISHED);
            transaction.Commit();
        }
    }
  }
  catch (Exception exportEx)
  {
    LogError(exportEx);// create a log class for cross-cutting concerns 
        ChangeStatus(MessageStatus.ERROR);
        AddErrorDescription(exportEx.Message);

    throw; // throw preserves original call stack for debugging/logging
  }
}

private void ChangeStatus(MessageStatus status)
{
  try
  {
    Status = status;
    srcDb.UpdateDataSet(drHeader.Table.DataSet, HEADERS,
        CreateHeaderInsertCommand(), CreateHeaderUpdateCommand(), null);
  }
  catch (Exception statusEx)
  {
   Log.Error(statusEx);
   throw;
  }
}

Also for any situation where you feel there are additional try/catch blocks needed, make them their own method if they are too ugly. I like Stefan Steinegger's answer, where the top level call in your app is the best place to catch.

Part of the problem here I imagine is that something is mutable that causes you to try to set the status after a failure. If you can factor your object to being in a consistent state whether or not your transaction works you don't have to worry about that part.