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);
        }
    }
}

Use using for everything implementing IDisposable and dispose it as fast as possible. — Tim Schmelter
– Tim Schmelter, Commented Aug 16, 2012 at 23:15
Slightly tangential to the question, but you can reduce the clutter of nested using blocks by extracting the inner chunk of code as a separate private method. This way the logic is not so deeply nested that it's hard to read, but you still have the correct outer 'using' blocks for disposal of resources. — Dan Bryant
– Dan Bryant, Commented Aug 16, 2012 at 23:18
@DanBryant Thanks, I actually have a Library Class where I handle all of this "Low Level" but to make my post easier to read and understand I went this route. — Mark Kram
– Mark Kram, Commented Aug 16, 2012 at 23:29
Just a quick comment - the exception catching is a bad idea. You should remove them from your code unless you've got them in purely for debugging and you plan to remove them later. — Enigmativity
– Enigmativity, Commented Aug 17, 2012 at 0:57

spender · Accepted Answer · 2012-08-16 23:12:06Z

11

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

answered Aug 16, 2012 at 23:12

spender

121k36 gold badges245 silver badges369 bronze badges

Sign up to request clarification or add additional context in comments.

2 Comments

Mark Kram Over a year ago

Great, I didn't think it would hurt.

Enigmativity Over a year ago

The GC never disposes objects. It just frees memory. You must explicitly dispose your objects either directly or with finalizers yourself.

Hand-E-Food · Accepted Answer · 2012-08-17 00:47:43Z

1

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))
    {
        ...
    }
}

answered Aug 17, 2012 at 0:47

Hand-E-Food

12.9k11 gold badges48 silver badges89 bronze badges

Comments

Enigmativity · Accepted Answer · 2012-08-17 01:27:25Z

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.

Collectives™ on Stack Overflow

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

3 Answers 3

2 Comments

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

2 Comments

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related