Where to put try/catch when using IDisposable

Question

I was just advised to put an entire using block inside a try, otherwise the using scope will prevent the exception from being caught. However, wouldn't that prevent the using from properly disposing its resources if an exception were thrown? If I have the code below, where should I put the try blocks?

using (connSQL = new SqlConnection(strConn)) 
{
    connSQL.Open();
    using (SqlCommand commSQL = new SqlCommand(strPreparedStatement, connSQL)) 
    {
        if (sqlParameters != null)
        {
            for (int i = sqlParameters.GetLowerBound(0); i <= sqlParameters.GetUpperBound(0); i++)
            {
                commSQL.Parameters.Add(sqlParameters[i]);
            }
        }
        drSQL = commSQL.ExecuteReader();
        dtReturn.Load(drSQL);

        commSQL.Parameters.Clear();
    }
}

In this application, it's far more important to ensure connections to the database don't start accumulating due to exceptions.

Answer 1

The using statement will already prevent connections from accumulating - it does the cleanup side of things for you, by effectively being a try/finally block with a call to Dispose in the finally block. If you also want a try/catch block, you can put it either inside or outside - but are you sure it shouldn't be at a higher level? How are you actually expecting to handle the exception?

As an aside, it's not clear why you're clearing the parameters from the command when the command is about to be disposed anyway...

I would encourage you to declare the variables within the using statements too, so that you don't end up trying to read from them outside the block:

using (SqlConnection connSQL = new SqlConnection(strConn)) {

In general, prefer to give your variables as narrow a scope as possible. Oh, and your SqlDataReader should be in a using statement too. It may not matter as you're closing the connection and the command anyway, but I would do it as a point of principle - it implements IDisposable, so you should dispose it.

Oh, and your way of iterating over sqlParameters is fairly longwinded at the moment. foreach makes it simpler, and even if it didn't I'd use for (int i = 0; i < sqlParameters.Length; i++) unless I had reason to believe it wasn't a "simple" array.

So, my equivalent code would look something like this:

using (SqlConnection connection = new SqlConnection(connectionString))
{
    connection.Open();
    using (SqlCommand command = new SqlCommand(strPreparedStatement, connection))
    {
        if (sqlParameters != null)
        {
            // If sqlParameter is an array, you can just use
            // command.Parameters.AddRange(sqlParameters) instead
            foreach (SqlParameter parameter in sqlParameters)
            {
                command.Parameters.Add(parameter);
            }
        }
        using (SqlDataReader reader = command.ExecuteReader())
        {
            DataTable table = new DataTable();
            // Perform any extra initialization here
            table.Load(reader);
            return table;
        }
    }
}

Answer 2

You need to wrap the data reader in a using statement as well since it is a disposable resource:

using (var connSQL = new SqlConnection(strConn)) 
using (var commSQL = connSQL.CreateCommand()) 
{
    connSQL.Open();
    commSQL.CommandText = strPreparedStatement;
    if (sqlParameters != null)
    {
        for (int i = sqlParameters.GetLowerBound(0); i <= sqlParameters.GetUpperBound(0); i++)
        {
            commSQL.Parameters.Add(sqlParameters[i]);
        }
    }
    using (var drSQL = commSQL.ExecuteReader())
    {
        dtReturn.Load(drSQL);
    }
}

I have also made the connection, command and data reader objects locally defined into this block scope.

As far as a try/finally statement is concerned you no longer need it as the using statements ensure that the Dispose method will be invoked even in the event of exceptions. And this method for sql connections and commands ensures to properly release them.