Inserting records into a database

Question

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

Answer 1

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

Answer 2

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

Answer 3

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

Answer 4

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.

Answer 5

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.

Answer 6

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.

Answer 7

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

Answer 8

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.

Answer 9

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.

Answer 10

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