Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

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

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);
    }
}
like image 381
lukem00 Avatar asked Jun 30 '10 10:06

lukem00


People also ask

Can you tell me what song is this?

Ask Google Assistant to name a song On your phone, touch and hold the Home button or say "Hey Google." Ask "What's this song?" Play a song or hum, whistle, or sing the melody of a song. Hum, whistle, or sing: Google Assistant will identify potential matches for the song.

How can you save a song from YouTube?

If you prefer to download YouTube audio only, you can also do that online by visiting youtube-mp3.org. Copy and paste the URL of the YouTube video you'd like to convert to audio on the text section on the website. Once it's pasted, click Convert Video to start the process.


3 Answers

  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.
like image 142
jgauffin Avatar answered Oct 24 '22 05:10

jgauffin


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.

Edit:

Because exception logging all over the place has at least these disadvantages:

  1. The same exception may be logged several times, which fills the log unnecessarily. It's hard to tell how many errors actually occurred.
  2. If the exception is caught and handled or ignored by the caller, there are still error messages in the log file, which is at least confusing.
like image 31
Stefan Steinegger Avatar answered Oct 24 '22 05:10

Stefan Steinegger


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....

like image 41
Matthew Abbott Avatar answered Oct 24 '22 06:10

Matthew Abbott