Exception handling placement - C#

Question

I've decided to remove some of the using statements in my code so I can catch specific exceptions and handle the disposing of resources manually. I have refactored some of the code to make it more readable and maintable, after implementing the new try/catch block I am left wondering if they have been placed correctly for the task at hand.

Example:

public static DataTable Select(string table, string[] column, Object operand)
{
        DataTable dTable = null;
        SQLiteConnection connection = null;
        SQLiteCommand command = null;
        SQLiteDataReader dReader = null;

        //convert to array for arguments
        StringBuilder query = new StringBuilder();
        query.Append("select ");

        for (int i = 0; i < column.Length; i++)
        {
            query.Append(column[i]);

            if (i < column.Length - 1)
            {
                query.Append(",");
            }
        }
        query.Append(" from ");
        query.Append(table);

        try
        {
            connection = new SQLiteConnection(_connectionString);
            command = new SQLiteCommand(query.ToString(), connection);
            dTable = new DataTable();

            connection.Open();

            dReader = command.ExecuteReader();

            dTable.Load(dReader);

            return dTable;
        }
        catch (SQLiteException sqle)
        {
            //Handle exception
        }
        finally
        {
            connection.Dispose();
            command.Dispose();
            dReader.Dispose();
            dTable.Dispose();
        }
        return null;
}

In this example I have only implemented the try/catch around the SQL operations themselves, I did this as it ensures any exceptions that are thrown can be noted and the resourced disposed correctly. I then noticed that this left the for loop open to exceptions, although the supplied indexer will be protected and created via a GUI.

Would I be wise to encapsulate the entire method in the try/catch statement or am I being overly cautious? You could say I'm looking for the best practice when it comes to managing the placement of the statements themselves.

Thanks for your time!

Edit:

I know that the using statement would be ideal in terms of handling the disposal and management of resources however as mentioned at the beginning of the question I wish to be able to catch specific types of exceptions, in particular those generated from the SQLite components.

Answer 1

Don't forget a null check:

finally {
  if (connection != null) connection.Dispose();
  if (command != null) command.Dispose();
  if (dReader != null) dReader.Dispose();
  if (dTable != null) dTable.Dispose();
}

It is possible that one of the constructors throws an exception, in that case the objects will not have been initialized.

Answer 2

Why use try/catch explicitly when what you are concerned about is resource management? Use using instead:

using(SQLiteConnection connection = new SQLiteConnection(_connectionString))
{ 
   ..
   using(SQLiteCommand command = new SQLiteCommand(query.ToString(), connection))
   {
      using(SQLiteDataReader  reader = dReader = command.ExecuteReader())
      {
          dTable.Load(dReader);
      }
   }
}

Also currently you are returning dTable, but you are disposing it in your finally block.

Answer 3

You could enclose the entire method in a try/catch if you think that there's any likelihood that exceptions will be thrown outside of the scope of database problems. You can easily differentiate the exceptions by what you catch. For example:

DataTable dTable = null;
SQLiteConnection connection = null;
SQLiteCommand command = null;
SQLiteDataReader dReader = null;

try
{
  // non-DB code

  // DB code
}
catch (SQLiteException sqle)
{
  // Handle DB exception
}
catch (IndexOutOfRangeException ie)
{
  // If you think there might be a problem with index range in the loop, for example
}
catch (Exception ex)
{
  // If you want to catch any exception that the previous catches don't catch (that is, if you want to handle other exceptions, rather than let them bubble up to the method caller)
}
finally
{
  // I recommend doing some null-checking here, otherwise you risk a NullReferenceException.  There's nothing quite like throwing an exception from within a finally block for fun debugging.
  connection.Dispose();
  command.Dispose();
  dReader.Dispose();
  dTable.Dispose();
}
return null;

Answer 4

A bigger problem you've introduced with this refactoring can be seen when you consider what will happen when the creation of one of the intermediate entities fails. For example, if the connection creation throws, then what happens to the exception thrown in your finally block that attemts to call Dispose on all those null variables? At least check for null beforehand, or put an additional try/catch inside your finally. I'd say you could benefit from the use of Resharper, which would have already pointed out these issues.

Answer 5

I don't know if it is best practice but I usually put try/catch statements around blocks of code where external resources are accessed like database access, file I/O, etc. which might spit out exceptions due to various reason (inaccessible resource, I/O error, etc.). I don't protect code which I'm in control of - that's where unit tests come into place

Btw: You know that you can replace your loop with string.Join()?

Update: Just to clarify: try/catch blocks only really make sense if you want to catch specific exception and execute some custom logic. Otherwise you should stick to using and let the exception bubble up and deal with it at the appropriate place (e.g. notify the user that some data could not be saved because the server is unavailable or so)

Answer 6

If all you are worried about is properly disposing of things, you should be using a 'using' block for this. http://msdn.microsoft.com/en-us/library/yh598w02.aspx