6
\$\begingroup\$

I wrote the following code to insert records into a database. The user selects the rows from the RadGrid and the insert command is executed when that user clicks the button. I've been spending a lot of time trying to make this work (nothing currently happens when I click the button). However, I just realized my approach is probably very inefficient due to the fact that it will result in the application repeatedly opening and closing database connections. Even though I'm dealing with only a few hundred records I'd like to follow best practices as much as possible.

How would you rewrite this code to be more efficient? My gut tells me it would be better to gather all of the new records and then deal with them as a collection.

    protected void RadButton2_Click(object sender, EventArgs e)
{
    foreach (GridDataItem item in RadGrid1.SelectedItems)
    {
        //GridDataItem item = (GridDataItem)RadGrid1.SelectedItems;
        int UserID = Convert.ToInt16(item["UserID"].Text);
        string Type = "D";
        DateTime Date = DateTime.Now;
        string Description = "Monthly Storage Fee - Tag: " + item["PackageTag"].Text + Label3.Text;
        Int32 AmountDue = Convert.ToInt32(item["AmtDue"].Text);


        string connectionString = ConfigurationManager.ConnectionStrings["Foo"].ConnectionString;
        SqlConnection connection = new SqlConnection(connectionString);

        try
        {

            SqlCommand cmd = new SqlCommand("INSERT INTO Billing (UserID, Type, Date, Description, Amount) VALUES (@UserID, @Type, @Date, @Description, @AmountDue)", connection);
            cmd.Parameters.AddWithValue("@UserID", UserID);
            cmd.Parameters.AddWithValue("@Type", Type);
            cmd.Parameters.AddWithValue("@Date", Date);
            cmd.Parameters.AddWithValue("@Description", Description);
            cmd.Parameters.AddWithValue("@AmountDue", AmountDue);

            connection.Open();
            cmd.ExecuteNonQuery();
        }

        catch
        {
            Label4.Text = "uh oh";
        }

        finally
        {
            connection.Close();
        }
\$\endgroup\$
  • 2
    \$\begingroup\$ ADO.NET will actually not dispose or open/close the connection behind the scenes but decides whether to reuse an existing connection with enabled Connection Pooling(default). So yes, it's recommendet to open/close connections or use the using-keyword. \$\endgroup\$ – Tim Schmelter Jun 28 '11 at 13:47
  • \$\begingroup\$ Are you by chance connecting to a SQL Server 2008 database? If so, you could work with the database developers to create a stored procedure that takes a table variable as an input parameter and inserts all the rows into the database at the same time. \$\endgroup\$ – Jeff Jun 28 '11 at 14:22
  • \$\begingroup\$ Yes I am actually. Interesting suggestion. I'm going to investigate that. For this project I am the database developer too. \$\endgroup\$ – hughesdan Jun 28 '11 at 14:39
  • \$\begingroup\$ Added some articles to get you started with TVP's as an answer. \$\endgroup\$ – Jeff Jun 28 '11 at 15:57

10 Answers 10

6
\$\begingroup\$

This is not best practice but simple logic.

Why you have add inside the loop this (non change) parameters ?

    string Type = "D";
    DateTime Date = DateTime.Now;
 string connectionString = 
       ConfigurationManager.ConnectionStrings["Foo"].ConnectionString;

Get them out of the loop.

Second place the connection inside using, and totally outside the loop, why you need to open it and close it all the time.

protected void RadButton2_Click(object sender, EventArgs e)
{
    //GridDataItem item = (GridDataItem)RadGrid1.SelectedItems; 
    string Type = "D";
    DateTime Date = DateTime.Now;

    string connectionString = ConfigurationManager.ConnectionStrings["Foo"].ConnectionString;
    SqlConnection connection = new SqlConnection(connectionString);

    using(SqlConnection connection = new SqlConnection(connectionString))
    {
        connection.Open();
        foreach (GridDataItem item in RadGrid1.SelectedItems)
        {
            try
            {
                string Description = "Monthly Storage Fee - Tag: " + item["PackageTag"].Text + Label3.Text;
                Int32 AmountDue = Convert.ToInt32(item["AmtDue"].Text);
                int UserID = Convert.ToInt16(item["UserID"].Text);

                using (SqlCommand cmd = new SqlCommand("INSERT INTO Billing (UserID, Type, Date, Description, Amount) VALUES (@UserID, @Type, @Date, @Description, @AmountDue)", connection))
                {
                    cmd.Parameters.AddWithValue("@UserID", UserID);
                    cmd.Parameters.AddWithValue("@Type", Type);
                    cmd.Parameters.AddWithValue("@Date", Date);
                    cmd.Parameters.AddWithValue("@Description", Description);
                    cmd.Parameters.AddWithValue("@AmountDue", AmountDue);

                    cmd.ExecuteNonQuery();
                }
            }
            catch
            {
                Label4.Text = "uh oh";
            }
        }    
    }
}

Need some extra test for check if the connection is open or fail

\$\endgroup\$
  • \$\begingroup\$ Aristos, can you spot anything I may have done wrong? When I run the app nothing actually inserts into the database. I'm not noticing any exceptions either. edit: I'm going with your recommended code by the way. \$\endgroup\$ – hughesdan Jun 28 '11 at 14:41
  • \$\begingroup\$ @hughesdan I belive that fail in the date, you need to format it in the SQL way, or declare that is a date. As you add it now, its convert it to string, and probably fails to insert. \$\endgroup\$ – Aristos Jun 28 '11 at 15:02
  • \$\begingroup\$ That's probably what it is. And come to thing of it I could also use SQL GetDate(). \$\endgroup\$ – hughesdan Jun 28 '11 at 16:45
  • 1
    \$\begingroup\$ SqlCommand also implements IDisposable and should be placed within a using block. \$\endgroup\$ – Jesse C. Slicer May 21 '15 at 17:47
  • \$\begingroup\$ @JesseC.Slicer Yes you have right, if you like please update the code. \$\endgroup\$ – Aristos May 22 '15 at 8:31
2
\$\begingroup\$

As long as you have access to SQL Server 2008, you can take advantage of TVP (Table Valued Parameters) which is a feature that was added to the 2008 version.

Here are two articles that will help you get started. The first article has one key piece of information that's easy to miss when you're first getting started with TVP's. You not only have to define the table type, you have to grant execute rights to the new TVP type:

  1. http://www.sqlteam.com/article/sql-server-2008-table-valued-parameters
  2. http://blog.sqlauthority.com/2008/08/31/sql-server-table-valued-parameters-in-sql-server-2008/

Please note that while TVP's themselves are readonly, you can use them to directly insert into a table which will give you even greater protection from SQL injection and will reduce the number of round trips to and from the database in order to insert a small batch of records into your database.

Hope this helps!

Jeff

\$\endgroup\$
1
\$\begingroup\$

Something like this: you'll need only 1 connection in this case

protected void RadButton2_Click(object sender, EventArgs e)
{
    string connectionString = ConfigurationManager.ConnectionStrings["Foo"].ConnectionString;
    SqlConnection connection = new SqlConnection(connectionString);
    connection.Open();
try
{
foreach (GridDataItem item in RadGrid1.SelectedItems)
{
    //GridDataItem item = (GridDataItem)RadGrid1.SelectedItems;
    int UserID = Convert.ToInt16(item["UserID"].Text);
    string Type = "D";
    DateTime Date = DateTime.Now;
    string Description = "Monthly Storage Fee - Tag: " + item["PackageTag"].Text + Label3.Text;
    Int32 AmountDue = Convert.ToInt32(item["AmtDue"].Text);



    try
    {

        SqlCommand cmd = new SqlCommand("INSERT INTO Billing (UserID, Type, Date, Description, Amount) VALUES (@UserID, @Type, @Date, @Description, @AmountDue)", connection);
        cmd.Parameters.AddWithValue("@UserID", UserID);
        cmd.Parameters.AddWithValue("@Type", Type);
        cmd.Parameters.AddWithValue("@Date", Date);
        cmd.Parameters.AddWithValue("@Description", Description);
        cmd.Parameters.AddWithValue("@AmountDue", AmountDue);
        cmd.ExecuteNonQuery();
    }

    catch
    {
        Label4.Text = "uh oh";
    }

    finally
    {

    }
}
}
catch
{

}
finally
{
 connection.Close();
}
\$\endgroup\$
1
\$\begingroup\$

ADO.NET will utilize Connection pooling if the connection string itself is identical each time, and will remove the overhead associated with the network.

The advantages of opening/closing a connection during operations like this is that if your code will do time consuming stuff, then the connection will be available to other threads in the application, and lessen the number of simultaneous connection that you use. Of course, it all depends on design.

You could either open close it before the loop, or do what you are doing. I would say that if your iterations are in the range of milliseconds, then open and close it once for the whole loop to minimize the slight overhead as well as object instantiation and memory usage.

\$\endgroup\$
1
\$\begingroup\$
  1. I don't think it is a good idea to have the database calling code in the UI page. See here
  2. Have you considered creating a data-layer using Entity Framework (or Linq2SQL). Then instead of writing all the queries manually, you can deal with the typed objects.
  3. While you should do what Aristos pointed out in any case (Even with EF, you should create the context at the level suggested by Aristos for the connection)
  4. After that perhaps you can use System.Threading.Tasks or a background worker to do the saving in a separate thread.
  5. I can see you are doing string concatenation with "+", consider using string.concat or string.format.
  6. Instead of using Convert.ToInt32(item["AmtDue"].Text) consider using int.TryParse() pattern to convert text to int.
  7. Consider using FxCop to detect potential issues in the code.
\$\endgroup\$
1
\$\begingroup\$

One other thing I noticed when looking at the other answers is that not only were you building the connection within the inner loop, you were building the query command multiple times as well.

It's been a while since I've done much non-SQL coding, but I think what you're looking for should be more along the following lines. You shouldn't have to clear out your parameter list between calls. Also, if the UserID is the same on your form for each row, I would only do that conversion once as well.

I've tried to extend the example Aristos provided by pulling even more out of the foreach loop and only creating the SqlCommand once to use between all of the calls.

protected void RadButton2_Click(object sender, EventArgs e)
{
    try
    {
        string Type = "D";
        DateTime Date = DateTime.Now;

        string connectionString = ConfigurationManager.ConnectionStrings["Foo"].ConnectionString;
        SqlConnection connection = new SqlConnection(connectionString);
        connection.Open();

        SqlCommand cmd = new SqlCommand("INSERT INTO Billing (UserID, Type, Date, Description, Amount) VALUES (@UserID, @Type, @Date, @Description, @AmountDue)", connection);

        cmd.Parameters.Add("@UserID", SqlDbType.Int);
        cmd.Parameters.Add("@Type", SqlDbType.Int);
        cmd.Parameters.Add("@Date", SqlDbType.Datetime);
        cmd.Parameters.Add("@Description", SqlDbType.NChar, 80);
        cmd.Parameters.Add("@AmountDue", SqlDbType.Money);

        cmd.Parameters("@Type").Value = Type;
        cmd.Parameters("@Date").Value = Date;

        foreach (GridDataItem item in RadGrid1.SelectedItems)
        {
            //GridDataItem item = (GridDataItem)RadGrid1.SelectedItems;
            int UserID = Convert.ToInt16(item["UserID"].Text);
            string Description = "Monthly Storage Fee - Tag: " + item["PackageTag"].Text + Label3.Text;
            Int32 AmountDue = Convert.ToInt32(item["AmtDue"].Text);

            //Change only the parameters that are different between calls.
            cmd.Parameters.("@UserID", UserID);  
            cmd.Parameters("@Description", Description);
            cmd.Parameters("@AmountDue", AmountDue);

            cmd.ExecuteNonQuery();
        }
    }
    catch
    {
        Label4.Text = "uh oh";
    }
    finally
    {
        connection.Close();
    }
}

I would still recommend utilizing Table Valued Parameters if possible, but this should be more efficient on your database if you weren't able to use TVP's.

\$\endgroup\$
  • \$\begingroup\$ That's a great point Jeff. No reason to loop through the parameters. I noticed you also added the SqlDbTypes, which I had omitted. Thanks. I'm going to experiment with a version of this page that uses TVP's. They look very promising. \$\endgroup\$ – hughesdan Jun 29 '11 at 13:34
1
\$\begingroup\$

Best practices are based on years of use. One thing bugs my eyes though: The SQL is a one liner:

INSERT INTO Billing (UserID, Type, Date, Description, Amount) VALUES (@UserID, @Type, @Date, @Description, @AmountDue)

Very hard to comprehend when scripts grow and I normally format an insert this way (easy to insert and remove fields, and the params can be copied directly):

INSERT INTO Billing 
(UserID
,Type
,Date
,Description
,Amount)
VALUES
(@UserID
,@Type
,@Date
,@Description
,@AmountDue)

.. making it easy to modify. I have done this in Delphi where the query-class is a TStrings.

I don't do want to do as below, as it is destined to fail when you forget a space.

"INSERT INTO Billing " +
"(UserID "+
",Type "+
",Date "+
",Description "+
",Amount) "+
"VALUES "+
"(@UserID "+
",@Type "+
",@Date "+
",@Description "+
",@AmountDue)";

The solution, that looks a lot like Delphi, is this:

List<string> l = new List<string>();
l.Add("INSERT INTO Billing");
l.Add("(UserID");
l.Add(",Type");
l.Add(",Date");
l.Add(",Description");
l.Add(",Amount)");
l.Add("VALUES");
l.Add("(@UserID");
l.Add(",@Type");
l.Add(",@Date");
l.Add(",@Description");
l.Add(",@AmountDue)");
string line = string.Join("\n",l.ToArray());

Of course this will be wrapped in a generic SQL-class

\$\endgroup\$
0
\$\begingroup\$

The insert block itself is alright: it safe and clean. And it avoids SQL Injection.

However, you should consider moving your persistent code away from the UI code. Try working in layers. There are some design patterns about the subject you should take a look.

\$\endgroup\$
  • \$\begingroup\$ Thanks. I plan to add layers for data access and business logic later. For the moment it's just simpler for me to work with fewer layers as I'm still learning. \$\endgroup\$ – hughesdan Jun 28 '11 at 14:14
0
\$\begingroup\$

I don't see anything particular that you've done wrong. Re: your question about opening and closing a connection multiple times, I think there is actually less overhead in doing this vs. leaving a connection open, as it does not fully allow the server to manage connection pooling.

And re: trying to batch these into one update/insert statement, I think this is more trouble than its worth. I've done that in the past, but it ends up being very painful (typically have to implement a solution like XML), and it doesn't yield much benefits. And if you look at more modern solutions, like ORMs, this is exactly how they implement it.

The only thing you may need to consider, is if all of these records need to be wrapped in a transaction (all records need to be updated/inserted, or all need to be rolled back). If so, then you would need to alter it for that. But if not, then what you have, to me, is perfectly acceptable.

\$\endgroup\$
  • \$\begingroup\$ Each insert is independent (they're only related from a workflow standpoint), so probably not necessary to wrap them in a transaction. Thanks for sharing your experience trying to batch transactions in the past. Have you tried batching them into a Data Table? I don't even know if that would work. I'm just reading about it now. \$\endgroup\$ – hughesdan Jun 28 '11 at 13:55
0
\$\begingroup\$

My take on it:

protected void RadButton2_Click(object sender, EventArgs e)
{

string connectionString = ConfigurationManager.ConnectionStrings["Foo"].ConnectionString;
SqlConnection connection = new SqlConnection(connectionString);
SqlCommand cmd = new SqlCommand("INSERT INTO Billing (UserID, Type, Date, Description, Amount) VALUES (@UserID, @Type, @Date, @Description, @AmountDue)", connection);

    try
    {
        connection.Open();

        foreach (GridDataItem item in RadGrid1.SelectedItems)
        {
            GridDataItem item = (GridDataItem)RadGrid1.SelectedItems;
            int UserID = Convert.ToInt16(item["UserID"].Text);
            string Type = "D";
            DateTime Date = DateTime.Now;
            string Description = "Monthly Storage Fee - Tag: " + item["PackageTag"].Text + Label3.Text;
            Int32 AmountDue = Convert.ToInt32(item["AmtDue"].Text);

            cmd.Parameters.Clear();
            cmd.Parameters.AddWithValue("@UserID", UserID);
            cmd.Parameters.AddWithValue("@Type", Type);
            cmd.Parameters.AddWithValue("@Date", Date);
            cmd.Parameters.AddWithValue("@Description", Description);
            cmd.Parameters.AddWithValue("@AmountDue", AmountDue);

            cmd.ExecuteNonQuery();
        }
    }
    catch
    {
        Label4.Text = "uh oh";
    }

    finally
    {
        connection.Close();
    }
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.