For loop only writing to my mssql stored procedure once?

Question

I have a for loop that collects the data from my C# web form and writes each item that it finds to a database via a stored procedure.

The problem I'm having is that it's writing to the database only once. I have stepped through the code in visual studio and inserted test variables to check that all the data is there and is being captured, which it is. Also because the stored procedure is executing correctly the first time I know that it's working.

So I think the problem might be with how I've got the try catch in the for loop?
Or possibly something else entirely - I could really do with a fresh pair of eyes and someone to point me in the right direction!

protected void log_hd_number()
{
    ////write results to DB.
    SqlConnection conn = new SqlConnection("Data Source=;Initial Catalog=;Integrated Security=True");
    SqlCommand cmd = conn.CreateCommand();
    SqlDataReader reader;
    cmd.CommandType = CommandType.StoredProcedure;
    cmd.CommandText = "insert_requested_hd";

    Dictionary<String, String> hdSize = new Dictionary<String, String>();

    hdSize.Add("hardDiskSizeData1", hardDiskSizeData1.Text);
    hdSize.Add("hardDiskSizeData2", hardDiskSizeData2.Text);

    int numberRequested = 2;

    for (int i = 1; i <= numberRequested; i++)
    {
        cmd.Parameters.AddWithValue("@hd_size", hdSize["hardDiskSizeData" + i]);
        cmd.Parameters.AddWithValue("@number_requested", numberRequested);
        cmd.Parameters.AddWithValue("@vm_id", 15);

        try
        {
            conn.Open();
            reader = cmd.ExecuteReader();
            reader.Close();
        }
        catch (Exception exc)
        {

        }
        finally
        {
            if (conn.State != ConnectionState.Closed)
                conn.Close();
        }
    }
}

EDIT:

SP:

ALTER PROCEDURE [dbo].[insert_requested_hd] 
    -- Add the parameters for the stored procedure here
    @hd_size nvarchar(150),
    @number_requested int,
    @vm_id int
AS
BEGIN
    -- SET NOCOUNT ON added to prevent extra result sets from
    -- interfering with SELECT statements.
    SET NOCOUNT ON;

    -- Insert statements for procedure here
    INSERT INTO dbo.hard_disk_size
                (
                    hd_size,
                    number_requested,
                    vm_id
                )
    VALUES 
                (
                    @hd_size,
                    @number_requested,
                    @vm_id
                )

Have you debugged the code line by line? checking the value of "i" in for loop? — Hannan Ayub, Jul 25 '17 at 10:19
Why are you doing ExecuteReader()? You just want ExecuteNonQuery() — Jonathan Willcock, Jul 25 '17 at 10:21
@hannan yes I have it goes through the right number of times — hlh3406, Jul 25 '17 at 10:21
the empty catch would silently ignore any exceptions. maybe there is a key violation or something of the sort, and goes unnoticed. set a breakpoint in the catch or output ex.ToString() in the catch. — Cee McSharpface, Jul 25 '17 at 10:21
try without closing the connection in between. open it before the loop, close it after the loop. I don't know (and therefore this is a comment) if this may have any side effects on the command, which is used repeatedly, and also it is not necessary. — Cee McSharpface, Jul 25 '17 at 10:24
@dlatikay just tried that thanks, but it didn't work unfortunately :( — hlh3406, Jul 25 '17 at 10:26
@dlatikay no exceptions. Will add the SP to the question now :) — hlh3406, Jul 25 '17 at 10:28
@dlatikay my mistake there is an exception! I hadn't put a breakpoint on it: {"Procedure or function insert_requested_hd has too many arguments specified."} - adding SP now — hlh3406, Jul 25 '17 at 10:31
Do you understand now that your error "handling" code with empty `catch` blocks is, in fact, error *hiding* code. Imagine a more subtle issue that only intermittently caused errors here - people would be reporting that data was wrong but you'd have no indications anywhere that a problem had occurred since you silently swallow exceptions and merrily carry on working as if everything is correct. Remove such catch blocks from your code and your (mental) toolbox. — Damien_The_Unbeliever, Jul 25 '17 at 10:36
@Damien_The_Unbeliever yes thanks - will always be putting proper error handling in place from now on — hlh3406, Jul 25 '17 at 10:38

score 6 · Accepted Answer · edited Aug 23 '17 at 07:30

6

you keep adding parameters to cmd in the loop without ever clearing the old ones. Maybe that's the issue.

also i'm not sure you can open a conn after it has been closed. i think you have to make a new one.

edited Aug 23 '17 at 07:30

Sneftel

40,271
12
71
104

answered Jul 25 '17 at 10:29

FS'Wæhre

196
2
16

^ this. of course. add once before the loop, then just keep setting the new values to the existing parameters. – Cee McSharpface Jul 25 '17 at 10:31
Also, @hlh3406 you catch exception but you do nothing about so most likely it fails for reasons pointed here but you do not see that because you have hidden it. – Jul 25 '17 at 11:11

score 0 · Answer 2 · answered Jul 25 '17 at 10:39

Calling a stored procedure in a loop is not a good idea, I guess.

If it is MS SQL Server, use user-defined table type and table-valued parameter!

This allows you to call the stored procedure once.

How to Save Object Graph in Master-Detail Relationship with One Stored Procedure.

score -1 · Answer 3 · answered Jul 25 '17 at 10:22

-1

You should be using

cmd.ExecuteNonQuery();

Instead of

reader = cmd.ExecuteReader();

answered Jul 25 '17 at 10:22

Hannan Ayub

378
2
15

likely, but the procedure could return a rowset, which in OP case would simply be discarded. Does a call to `ExecuteReader` fail in connection with `CommandType.StoredProcedure`, when there is no rowset? – Cee McSharpface Jul 25 '17 at 10:26

André Pedroso · Answer 4 · 2017-07-25T10:46:06.477

-1

You are closing your connection after the first loop. If you really want to go to DB every loop you have to open a new connection for each round.

Either way, I think you shouldn't hit the DB so many times. Have you think about BulkInsert? Take a look here

edited Jul 25 '17 at 10:46

answered Jul 25 '17 at 10:30

André Pedroso

1,574
1
17
16

For loop only writing to my mssql stored procedure once?

4 Answers4