C# Connection And NonQuery Execution To Database

Question

Just wondering if there are any typical issues when using this form of database command/connection. Is there something "better"? Anything else that could possibly help me improve my TSQL/C# skills would be much appreciated! Thank you!

private void Approval_Status(object sender, EventArgs e)
    {
        Button Approval = (Button)sender;
        /*
         * Boolean determining if the request was approved or denied
         */
        Boolean Status = false;

        if (ValidateApproval(Approval.Text.Trim().ToUpper()) == true)
        {
            SqlCommand cmd0 = new SqlCommand();
            cmd0.Connection = db.con(user.Authority);
            cmd0.CommandType = CommandType.Text;
            cmd0.CommandText = "UPDATE [TBL_REQUEST] " +
                "SET [TBL_REQUEST].[REQUEST_STATUS]=@Status, [TBL_REQUEST].[APPROVED_BY]=@Approver, " +
                "[TBL_REQUEST].[DATE_APPROVED]=@Date, [TBL_REQUEST].[PRINTED_NAME]=@Name, " +
                "[TBL_REQUEST].[TITLE]=@Title, [TBL_REQUEST].[PTO_USED]=@Used " +
                "WHERE [TBL_REQUEST].[ID]=@ID; ";
            if (Approval.Text.ToUpper() == codes.RequestApproved)
            {
                cmd0.Parameters.AddWithValue("@Status", SqlDbType.VarChar).Value = codes.RequestApproved;
                Status = true ;
            }
            else
            {
                cmd0.Parameters.AddWithValue("@Status", SqlDbType.VarChar).Value = codes.RequestDenied;
                Status = false;
            }
            cmd0.Parameters.AddWithValue("@Approver", SqlDbType.VarChar).Value = user.User;
            cmd0.Parameters.AddWithValue("@Date", SqlDbType.Date).Value = DateTime.Today.ToShortDateString();
            cmd0.Parameters.AddWithValue("@Name", SqlDbType.VarChar).Value = txtApproval.Text.Trim();
            cmd0.Parameters.AddWithValue("@Title", SqlDbType.VarChar).Value = user.Title;
            cmd0.Parameters.AddWithValue("@Used", SqlDbType.Float).Value = (float)nudUsed.Value;
            cmd0.Parameters.AddWithValue("@ID", SqlDbType.VarChar).Value = txtID.Text.Trim();
            /*
             * Execute our non-query
             */
            db.conEstablished.Open();
            cmd0.ExecuteNonQuery();
            db.conEstablished.Close();
            /*
             * Dispose our resources
             */
            cmd0.Dispose();
            ClearRequestsPanel();
            /*
             * Inform our user of a successful update
             */
            if (Status == true)
            {
                MessageBox.Show(msg.RequestApproved);
            }
            else if (Status == false)
            {
                MessageBox.Show(msg.RequestDenied);
            }
        }

What is `db.con`? And the `using` statement is part of the language - it's not a function. — Jon Skeet, Jan 02 '13 at 20:15
The typical issue when using this form is forgetting the `cmd0.Dispose()` call, which `using` takes care of. :) — prprcupofcoffee, Jan 02 '13 at 20:15
@David: Not just forgetting it, but failing to put it in a `finally` block... — Jon Skeet, Jan 02 '13 at 20:20
Please remember that `x == true => x`. You don't need to compare boolean expression to `true`. — Ilia G, Jan 02 '13 at 20:30
@IliaG Personally I'm ok with badly named booleans going with that form since `if(Status)` reads slightly worse than `if (Status==true)` Personally I would have rather see well named booleans so it reads nicely `if(IsApproved)` or `if(Approved)` — Conrad Frix, Jan 02 '13 at 22:21

score 4 · Answer 1 · answered Jan 02 '13 at 20:17

It looks like you may well be trying to create your own connection pooling. Don't do that:

Use using statements for all disposable resources. That includes SqlConnection and SqlCommand. That way, even if an exception is thrown, the resource is disposed
Create a new SqlConnection for each database operation, and let the system manage pooling the real network connections.

It's not clear what db.con(...) and db.conEstablished are, but it sounds like you've quite possibly got a single connection - which means that you can't use this code safely in a multi-threaded environment. It's fine to have a helper method to create a SqlConnection, but it should just create a new one each time, which is then disposed when the operation has been completed.

Additionally, you should start following .NET naming conventions, and this code:

if (Status == true)
{
    MessageBox.Show(msg.RequestApproved);
}
else if (Status == false)
{
    MessageBox.Show(msg.RequestDenied);
}

... would be better written as:

MessageBox.Show(Status ? msg.RequestApproved : msg.RequestDenied);

MessageBox.Show(Status ? msg.RequestApproved : msg.RequestDenied); ...Was helpful! Learned something new, thanks! — Joshua Volearix, Jan 02 '13 at 20:26
@JoshuaVolearix: Even if you wanted to use the `if` version, it would be better to use `if (Status) { ... } else { ... }`. For one thing, if it's not true it will definitely be false - and it's generally cleaner style IMO to use `if (foo)` or `if (!foo)` rather than `if (foo == true)` or `if (foo == false)`. — Jon Skeet, Jan 02 '13 at 20:37
Definitely good to know, I've only been programming C# for two weeks now and everything else past the ABSOLUTE basics has been self taught in general. Things like that don't really get covered in beginner books! — Joshua Volearix, Jan 02 '13 at 20:48

score 2 · Answer 2 · answered Jan 02 '13 at 20:23

using is the "proper" way to handle disposable objects like the SqlConnection.

Part of the problem with your code is that if the query causes an exception, then this line will be skipped, because the exception will break out of the method:

cmd0.Dispose();

When using using the dispose will always be called, even if an exception exits the block (internally it just wraps the code in a try/catch and puts the call to .Dispose() in the catch block.)

Also you should note that the SqlConnection class handles pooling internally. It is not actually a single open network connection to the DB.

score 0 · Accepted Answer · answered Jan 03 '13 at 10:54

Your code should look something like this. Obviously I've got variables here that are defined elsewhere but hopefully this gives you a better idea of how to use using

using(var dbconn = new SqlConnection(connectionString))
{
    using (var dbcmd = new SqlCommand(storedProcedure, dbconn))
    {
        dbcmd.CommandType = CommandType.StoredProcedure;
        dbcmd.Parameters.AddRange(sqlParameters.ToArray());
        dbconn.Open();
        return dbcmd.ExecuteNonQuery();
    }
}

Thank you for actually answering the question. I was looking for the proper syntax for 'using'. — Joshua Volearix, Jan 03 '13 at 14:39

C# Connection And NonQuery Execution To Database

3 Answers3