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);
}
}
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.
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.
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:
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:
These are just the things I do personally, a lot of developers prefer doing it ways in which they are comfortable....
If you love us? You can donate to us via Paypal or buy me a coffee so we can maintain and grow! Thank you!
Donate Us With