Wrapping a Using Block inside of another Using Block - is it overkill?

Question

I have a section of code in my project where I an wrapping a Using Block Inside of Another Using Block, I am wondering is this a good practice or just overkill (Please note I understand that this is a very simplistic snippet of code, it was used for illustration purposes only):

protected void Submit_Click(object sender, EventArgs e)
    {
        try
        {
            using (SqlConnection cn = new SqlConnection(ConfigurationManager.ConnectionStrings["RegConnectionString"].ConnectionString))
            {
                cn.Open();

                string cmdStr = "SELECT COUNT(*) FROM REGISTRATION WHERE UserName ='" + this.TextBoxUN.Text + "' ";
                using (SqlCommand selectUser = new SqlCommand(cmdStr, cn))
                {
                    int temp = Convert.ToInt32(selectUser.ExecuteScalar().ToString());

                    if (temp == 0)
                    {
                        string insCmd = "Insert INTO REGISTRATION (UserName, Password, EmailAddress, FullName, Country) VALUES (@UserName, @Password, @EmailAddress, @FullName, @Country)";
                        using (SqlCommand insertUser = new SqlCommand(insCmd, cn))
                        {
                            try
                            {
                                insertUser.Parameters.AddWithValue("@UserName", this.TextBoxUN.Text);
                                insertUser.Parameters.AddWithValue("@Password", this.TextBoxPass.Text);
                                insertUser.Parameters.AddWithValue("@EmailAddress", this.TextBoxEA.Text);
                                insertUser.Parameters.AddWithValue("@FullName", this.TextBoxFN.Text);
                                insertUser.Parameters.AddWithValue("@Country", this.DropDownListCountry.SelectedItem.ToString());

                                insertUser.ExecuteNonQuery();
                                Response.Redirect("~/Login.aspx");
                            }
                            catch (Exception ex)
                            {
                                Response.Write(ex.Message);
                            }
                        }
                    }
                    else
                    {
                        Response.Write("User already Exists in Database");
                    }
                }
            }
        }
        catch (Exception ex)
        {
            Response.Write(ex.Message);
        }
    }
}

Answer 1

Yes. Good practice. Dispose of stuff in the smallest scope possible, otherwise you're leaving it to GC to do quite some time later.

Answer 2

Absolutely you can nest using statements. Each using statement only applies to one object, so if several require disposal, use several using statements.

Additionally, you could reduce the nesting by closing the first command before opening the second.

int temp;
using (SqlCommand selectUser = new SqlCommand(cmdStr, cn))
{
    temp = Convert.ToInt32(selectUser.ExecuteScalar().ToString());
}
if (temp == 0)
{
    string insCmd = ...;
    using (SqlCommand insertUser = new SqlCommand(insCmd, cn))
    {
        ...
    }
}

Answer 3

I thoroughly agree that using using statements is a must when dealing with nested disposable objects.

However, I would suggest a further change to the code. To help make your code readable, testable and maintainable, it is a good idea to use function composition. Here's how I would change the main body of the code:

using (var cn = new SqlConnection(ConfigurationManager.ConnectionStrings["RegConnectionString"].ConnectionString))
{
    cn.Open();
    if (checkUserExists(cn, this.TextBoxUN.Text))
    {
        Response.Write("User already Exists in Database");
    }
    else
    {
        addUser(cn, this.TextBoxUN.Text, this.TextBoxPass.Text, this.TextBoxEA.Text, this.TextBoxFN.Text, this.DropDownListCountry.SelectedItem.ToString());
        Response.Redirect("~/Login.aspx");
    }
    cn.Close();
}

This code is more compact and easier to reason about what is going on.

Prior to this code you need to define the checkUserExists and addUser lamdbas, like so:

Func<SqlConnection, string, bool> checkUserExists = (cn, un) =>
{
    var query = "SELECT COUNT(*) FROM REGISTRATION WHERE UserName = @UserName";
    using (var command = new SqlCommand(query, cn))
    {
        command.Parameters.AddWithValue("@UserName", un);
        return Convert.ToInt32(command.ExecuteScalar().ToString()) != 0;
    }
};

Action<SqlConnection, string, string, string, string, string> addUser = (cn, un, pw, e, fn, c) =>
{
    string query = "Insert INTO REGISTRATION (UserName, Password, EmailAddress, FullName, Country) VALUES (@UserName, @Password, @EmailAddress, @FullName, @Country)";
    using (var command = new SqlCommand(query, cn))
    {
        command.Parameters.AddWithValue("@UserName", un);
        command.Parameters.AddWithValue("@Password", pw);
        command.Parameters.AddWithValue("@EmailAddress", e);
        command.Parameters.AddWithValue("@FullName", fn);
        command.Parameters.AddWithValue("@Country", c);

        command.ExecuteNonQuery();
    }
};

Each of these is also very straightforward and their intent is clear and easy to reason about.

Because they are lambdas they don't clog up your classes with unnecessary methods - it's all contained within the one method. Nice, neat & tidy.

Of course they all use using statements.

Hope this helps.