Question on how to correctly use SQL Connections

Question

I am new to C# and am building an app which interacts with and performs various operations against SQL databases.

Here is sample syntax of what I have been using for performing these tasks up to now (this populates a combo box with a list of databases for example)

private void button1_Click(object sender, EventArgs e)
{
   string connectionString = txtConnString.Text;
   SqlConnection conn = new SqlConnection(connectionString);
   try
   {
      string sql = "SELECT name FROM master.sys.databases;";
      conn.Open();
      SqlCommand cmd = new SqlCommand(sql, conn);
      SqlDataReader dr = cmd.ExecuteReader();
      while (dr.Read())
      {
         comboDatabaseList.Items.Add(dr[0]);
      }
      dr.Close();
      cmd.Dispose();
   }
   catch (SqlException ex)
   {
      MessageBox.Show(ex.Message, "Error", MessageBoxButtons.OK, MessageBoxIcon,Error);
   }
   finally
   {
      if (conn.State == ConnectionState.Open)
         conn.Close();
   }

}

However, as I'm learning and looking into things more, I understand it is best practice to wrap up SQL work in using blocks.

So having said that, would the following be more appropriate for the above, or is this overkill?

private void button1_Click(object sender, EventArgs e)
{
    string connectionString = txtConnString.Text;
    string sql = "SELECT name FROM master.sys.databases;";
    using (SqlConnection conn = new SqlConnection(connectionString)
    {
        try
        {
            conn.Open();
            using (SqlCommand cmd = new SqlCommand(sql, conn))
            {
                using (SqlDataReader dr = cmd.ExecuteReader())
                {
                    while (dr.Read())
                    {
                        comboDatabaseList.Items.Add(dr[0]);
                    }
                    dr.Close();
                }
                cmd.Dispose();
            }
        }
        catch (SqlException ex)
        {
            MessageBox.Show(ex.Message, "Error", MessageBoxButtons.OK, MessageBoxIcon,Error);
        }
        finally
        {
            if (conn.State == ConnectionState.Open)
                conn.Close();
        }
    }
}

I just want to make sure that the app is closing everything appropriately (including visual studio when I'm running debug to test the app), and the memory etc is returned to the system.

EDIT/UPDATE: Thanks for the responses everyone. So, in a nutshell, from what I am reading, if I use the bottom code block instead, because the SqlConnection is wrapped in an using block, I can remove the need for dr.Close() and cmd.Dispose() and I can also remove the finally from the try catch finally block?

Or can I just get rid of the try catch finally block altogether? I'm only using the catch to catch SQL errors, and the finally to close the connection, so if using is handling this, I don't need it do I?

I could just do this and simplify my code:

private void button1_Click(object sender, EventArgs e)
{
    string connectionString = txtConnString.Text;
    string sql = "SELECT name FROM master.sys.databases;";
    using (SqlConnection conn = new SqlConnection(connectionString)
    {
        conn.Open();
        using (SqlCommand cmd = new SqlCommand(sql, conn))
        {
            using (SqlDataReader dr = cmd.ExecuteReader())
            {
                while (dr.Read())
                {
                    comboDatabaseList.Items.Add(dr[0]);
                }
            }
        }
    }
}

Is that right?

Answer 1

Yes, you should wrap your SqlConnection with using. That means you can remove the finally block - disposing the connection will also close it, no need to so before.

Technically SqlCommands don't need to be disposed since they currently don't have any unmanaged resources, and disposing the SqlConnection will free up any resources used by SqlData Reader.

However, in my opinion it's good practice to always wrap any IDisposable object with using instead of relying an implementation detail like that, which could potentially change. Others may disagree and not see it as necessary. I don't think there's any harm in it, especially when using the colon syntax to avoid nested using indentation that may harm readability.

You can use the colon syntax

using SqlCommand cmd = new SqlCommand(sql, conn);

instead of

using (SqlCommand cmd = new SqlCommand(sql, conn)) {}

to lower the indentation level. When using the colon syntax, the dispose will be run at the end of the surrounding block