0

As shown in the screenshot, I was trying to insert data into a table. The 1st iteration works fine, but the 2nd iteration throws an error/exception.

What is wrong with my code?

Below is the code.

SqlConnection sqlconn = new SqlConnection(sqlconnectionstring);
//  sqlconn.Open();

string InsertData = "INSERT INTO AUStagAPITestData ([TestSuite], [TestCase],[Status], [Info], [Time], [IsArchived], [DateTime]) VALUES (@TestSuite, @TestCase, @Status, @Info, @Time, @IsArchived, @DateTime)";

SqlCommand Insertcmd = new SqlCommand(InsertData, sqlconn);

for (int j = 1; j < TDData.Length; j +=5)
{
    sqlconn.Open();

    string TestSuite = TDData[j];
    string TestCase = TDData[j+1];
    string Status = TDData[j + 2];
    string Info = TDData[j + 3];
    string Time = TDData[j + 4];

    Insertcmd.Parameters.AddWithValue("@TestSuite", TestSuite);
    Insertcmd.Parameters.AddWithValue("@TestCase", TestCase);
    Insertcmd.Parameters.AddWithValue("@Status", Status);
    Insertcmd.Parameters.AddWithValue("@Info", Info);
    Insertcmd.Parameters.AddWithValue("@Time", Time);
    Insertcmd.Parameters.AddWithValue("@IsArchived", "1");
    Insertcmd.Parameters.AddWithValue("@DateTime", DateTime.Now);

    Insertcmd.ExecuteNonQuery();
    sqlconn.Close();
}

enter image description here

2
  • Don't use a loop for insert. Use a table valued parameter instead. Commented Aug 5, 2017 at 5:41
  • You should check out Can we stop using AddWithValue() already? and stop using .AddWithValue() - it can lead to unexpected and surprising results... Commented Aug 5, 2017 at 7:13

6 Answers 6

2

What you really should do is:

  • create the list of parameter objects once, before the loop
  • during the loop, only set their values

Something like this:

string InsertData = "INSERT INTO AUStagAPITestData ([TestSuite], [TestCase],[Status], [Info], [Time], [IsArchived], [DateTime]) VALUES (@TestSuite, @TestCase, @Status, @Info, @Time, @IsArchived, @DateTime)";

// put your connection and command into *USING* blocks to properly dispose of them
using (SqlConnection sqlconn = new SqlConnection(sqlconnectionstring))
using (SqlCommand Insertcmd = new SqlCommand(InsertData, sqlconn))
{
    // create the parameters **ONCE** and define their datatypes
    // I have only *guessed* what the datatypes could be - adapt as needed
    Insertcmd.Parameters.Add("@TestSuite", SqlDbType.VarChar, 50);
    Insertcmd.Parameters.Add("@TestCase", SqlDbType.VarChar, 50);
    Insertcmd.Parameters.Add("@Status", SqlDbType.VarChar, 50);
    Insertcmd.Parameters.Add("@Info", SqlDbType.VarChar, 50);
    Insertcmd.Parameters.Add("@Time", SqlDbType.Time);
    Insertcmd.Parameters.Add("@IsArchived", SqlDbType.Boolean);
    Insertcmd.Parameters.Add("@DateTime", SqlDbType.DateTime);

    sqlconn.Open();

    // now loop over the data and set the parameter values
    for (int j = 1; j < TDData.Length; j +=5)
    {
        string TestSuite = TDData[j];
        string TestCase = TDData[j+1];
        string Status = TDData[j + 2];
        string Info = TDData[j + 3];
        string Time = TDData[j + 4];

        Insertcmd.Parameters["@TestSuite"].Value = TestSuite;
        Insertcmd.Parameters["@TestCase"].Value = TestCase;
        Insertcmd.Parameters["@Status"].Value = Status;
        Insertcmd.Parameters["@Info"].Value = Info;
        Insertcmd.Parameters["@Time"].Value = Time;
        Insertcmd.Parameters["@IsArchived"].Value = true;
        Insertcmd.Parameters["@DateTime"].Value = DateTime.Now;

        // execute the query in the loop
        Insertcmd.ExecuteNonQuery();
    }   

    sqlconn.Close();
}
Sign up to request clarification or add additional context in comments.

Comments

1

It's complaining you have already added:

Insertcmd.Parameters.AddWithValue("@TestSuite

The fix is to instantiate a new SqlCommand each iteration:

for (int j = 1; j < TDData.Length; j +=5) 
{
sqlconn.Open(); 
SqlCommand Insertcmd = new SqlCommand(InsertData, sqlconn); 
string TestSuite= TDData[j];

1 Comment

Or better yet: create the parameter objects once, before the loop, and then in the loop only set the values - not create new parameter objects over and over and over again ...
0

try to move sqlconn.Open() & sqlconn.Close() out of the for loop and renew sqlcommand object.

sqlconn.Open();
for (int j = 1; j < TDData.Length; j += 5)
{
    SqlCommand Insertcmd = new SqlCommand(InsertData, sqlconn);
    string TestSuite = TDData[j];
    ...
}
sqlconn.Close();

5 Comments

Only use resources as long as you need them, moving Open outside the loop is an example, connections are not actually opened and closed when you call SqlConnection.Open. ASP.NET recycles active connections from the pool when the conn string matches a previously used conn string. The overhead involved is inconsequential, and additionally, trying to "do it yourself" means you have to assume all the management tasks of ensuring the connection is still active for each subsequent use, which adds complexity and overhead. With connection pooling, best practice is to open and close it for every use.
I don't agree with you, the best practice it we shouldn't open/close connection in a for loop, we can reuse the connection.
Is that a personal opinion?
@JeremyThompson As far a I know the connection pool has max size, then if the for loop is 100,000 times, what do you think about it? please google the best practice for sqlconnection inside a loop.
If you're doing 5 inserts in a loop it doesn't really matter keeping open, however many more iterations and you are wrong. I wouldn't have written such a long comment urging you to check what you believe is best practice if I wasn't 100% sure, think about then find these so called articles saying Conn.Open outside loops is BP..
0

Check this example based on your code:

string connectionString, queryInsert;
string[] arrayData = new string[10];

connectionString = ConfigurationManager.ConnectionStrings["DbConn"].ConnectionString;
queryInsert = @"
    INSERT INTO AUStagAPITestData
    (
        [TestSuite], [TestCase], [Status], [Info], [Time], [IsArchived], [DateTime]
    )
    VALUES (
        @TestSuite, @TestCase, @Status, @Info, @Time, @IsArchived, @DateTime
    )
";

using (SqlConnection connection = new SqlConnection(connectionString))
using (SqlCommand command = new SqlCommand(queryInsert, connection))
{
    string testSuite, testCase, status, info, time;

    connection.Open();

    for (int j = 1; j < arrayData.Length; j += 5)
    {
        testSuite = arrayData[j];
        testCase = arrayData[j + 1];
        status = arrayData[j + 2];
        info = arrayData[j + 3];
        time = arrayData[j + 4];

        command.Parameters.AddWithValue("@TestSuite", testSuite);
        command.Parameters.AddWithValue("@TestCase", testCase);
        command.Parameters.AddWithValue("@Status", status);
        command.Parameters.AddWithValue("@Info", info);
        command.Parameters.AddWithValue("@Time", time);
        command.Parameters.AddWithValue("@IsArchived", "1");
        command.Parameters.AddWithValue("@DateTime", DateTime.Now);

        command.ExecuteNonQuery();

        // To Clear parameters
        command.Parameters.Clear();
    }
    // no need to close a disposed object since dispose will call close 
}

Comments

0

You can use Insertcmd.Parameters.Clear() inside your for loop.

Comments

0

As I wrote in the comments, I would use a stored procedure with a table valued parameter instead of inserting the records one by one. This has the advantage of only making one round trip from your application code to your database.
However, it also has a disadvantage - if one row fails for any reason (say, violating a constraint), the entire insert will fail.

Having said that, in order to use a table valued parameter you should first create a user defined table type. Note that I'm guessing your columns data types here, you might need to change them:

CREATE TYPE dbo.tt_TestData (
    [TestSuite] int, -- I'm guessing foreign keys
    [TestCase] int,
    [Status] int, 
    [Info] nvarchar(255), 
    [Time] time, 
    [IsArchived] bit, 
    [DateTime] datetime
);
GO

After you've done that, you can create your stored procedure:

CREATE PROCEDURE stp_AUStagAPITestData_Insert
(
    @Data dbo.tt_TestData READONLY -- Note: Readonly is a must!
)
AS

INSERT INTO AUStagAPITestData (
    [TestSuite], 
    [TestCase],
    [Status], 
    [Info], 
    [Time], 
    [IsArchived], 
    [DateTime]
)
SELECT 
    [TestSuite], 
    [TestCase],
    [Status], 
    [Info], 
    [Time], 
    [IsArchived], 
    [DateTime]
FROM @Data;
GO

Now, to execute this stored procedure using ADO.Net, you will need to create a data table for your data, and send it as a paramameter of type SqlDbType.Structured to the stored procedure:

using (var sqlconn = new SqlConnection(sqlconnectionstring))
{
    using (var Insertcmd = new SqlCommand("stp_AUStagAPITestData_Insert", sqlconn))
    {
        //  Create the data table
        using (var dt = new DataTable())
        {
            dt.Columns.Add("TestSuite", typeof(int));
            dt.Columns.Add("TestCase", typeof(int));
            dt.Columns.Add("Status", typeof(int));
            dt.Columns.Add("Info", typeof(string));
            dt.Columns.Add("Time", typeof(DateTime));
            dt.Columns.Add("IsArchived", typeof(bool));
            dt.Columns.Add("DateTime", typeof(DateTime));

            // Populate the data table from the TDData string array
            for (int j = 1; j < TDData.Length; j += 5)
            {
                dt.Rows.Add
                (
                    TDData[j],      // TestSuite
                    TDData[j + 1],  // TestCase
                    TDData[j + 2],  // Status
                    TDData[j + 3],  // Info
                    TDData[j + 4],  // Time
                    true,           // IsArchived
                    DateTime.Now    // DateTime
                );
            }

            Insertcmd.CommandType = CommandType.StoredProcedure;
            Insertcmd.Parameters.Add("@Data", SqlDbType.Structured).Value = dt;

            try
            {
                sqlconn.Open();
                Insertcmd.ExecuteNonQuery();
            }
            catch (Exception e)
            {
                // Exception handling code goes here...
            }
        }
    }
}

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.