Code Snippet Improvement for database call in .NET

Question

Can I get a list of issues with the code below, and fixes?

 string ProgramID = HttpContext.Current.Session[CommonFunctions.myNGconnectSessionVars.ProgramId].ToString();
            SqlConnection con = new SqlConnection(ConfigurationManager.AppSettings["MyNGConnectDashBoardConnectionString"].ToString());
            SqlCommand cmd = new SqlCommand();
            DataSet ds = new DataSet();
            try
            {
                cmd.Connection = con;
                cmd.CommandTimeout = 900;
                cmd.CommandText = "dsb_GetSubscriptionDetailsForSubscriber_ForValidations";
                cmd.CommandType = CommandType.StoredProcedure;
                cmd.Parameters.Clear();
                cmd.Parameters.AddWithValue("@GPCustomerID", strGPCustomerID);
                cmd.Parameters.AddWithValue("@UserName", strUserName);
                cmd.Parameters.AddWithValue("@ServerName", ConfigurationManager.AppSettings["ServerName"].ToString());
                //code changed by sushma 3/22/2011 as new tab added for reach
                //cmd.Parameters.AddWithValue("@ProgramID", CommonFunctions.ProgramID);
                cmd.Parameters.AddWithValue("@ProgramID", ProgramID);
                cmd.Parameters.AddWithValue("@IsTeacher", blnIsTeacher);
                // cmd.CommandTimeout = 0;
                SqlDataAdapter da = new SqlDataAdapter(cmd);
                da.Fill(ds);
            }
            catch (Exception ex)
            {
                throw ex;
            }
            finally
            {
                con.Close();
            }

            return ds;

Issue 1: this isn't a code review site. Issue 2: we're not here to do your work for you. Issue 3: there's not even a please in there. — Chris Eberle, Jan 05 '12 at 17:58
@Chris: my impression is that "please" is generally considered noise on SO. — Igby Largeman, Jan 05 '12 at 21:22
@IgbyLargeman not when someone is asking me to do their work for them. — Chris Eberle, Jan 05 '12 at 21:29
@Chris: yeah, not defending the lame question. "Can I get a..." is a bit of an affront to politeness. — Igby Largeman, Jan 05 '12 at 21:32

Darin Dimitrov · Answer 1 · 2012-01-05T18:05:03.550

The biggest issue that I can see is that you are not disposing disposable objects such as connections, commands, adapters, ... so in case of exception you might leak resources. To fix your code wrap all disposable resources in using statements:

using (SqlConnection con = new SqlConnection(ConfigurationManager.AppSettings["MyNGConnectDashBoardConnectionString"].ToString()))
using (SqlCommand cmd = con.CreateCommand())
{
    con.Open();
    cmd.CommandTimeout = 900;
    cmd.CommandText = "dsb_GetSubscriptionDetailsForSubscriber_ForValidations";
    cmd.CommandType = CommandType.StoredProcedure;
    cmd.Parameters.AddWithValue("@GPCustomerID", strGPCustomerID);
    cmd.Parameters.AddWithValue("@UserName", strUserName);
    cmd.Parameters.AddWithValue("@ServerName", ConfigurationManager.AppSettings["ServerName"].ToString());
    using (SqlDataAdapter da = new SqlDataAdapter(cmd))
    {
        DataSet ds = new DataSet();
        da.Fill(ds);
        return ds;
    }
}

I have also removed the try/catch block as you don't seem to be doing anything useful in the catch statement other than altering the stack trace which is bad.

score 2 · Accepted Answer · answered Jan 05 '12 at 18:01

Here are some immediate improvements

Use using statement rather than try/finally for the connection management

Use throw and not throw ex to re-throw the exception, better yet you can just remove the catch and re-throw

There is no need to call Parameters.Clear on the command that you just instantiated.

And finally, if you are going to do a lot of DB work, I would suggest you look at using a lightweight ORM.

score 1 · Answer 3 · answered Jan 05 '12 at 17:58

Except:

ConfigurationManager.AppSettings["ServerName"]

Maybe be a little slow when called hundreds of times and

strUserName

you are fired for using hungarian notation when strondly discouraged for the last 10 years?

And that:

cmd.CommandTimeout = 900;

Ah - no. 900 seconds? What the heck do you think you do there that needs that?

cmd.Parameters.Clear();

Yeah. Like this is needed on a new object.

DataSet ds = new DataSet();

And i fire anyone in general who uses datasets ;) Except in a general query tool where that thing geos straight to the UI.

Not a lot - well, also that we dont realyl do homework reviews here. And I rather would not use a useles try/catch instead going to USING statements.

score 1 · Answer 4 · answered Jan 05 '12 at 18:01

It's mainly one construct, I want to comment on:

Don't use throw ex - it will alter the StackTrace of ex. Use throw; instead.
Leave away the catch-Block. You can simply use try-finally
Use two using statements for con and cmd

Some other points:

use self-explanatory variable names
Put the change history into the version control system (and remove it from the comments!)
Let the source control system handle old code instead of simply commenting it out
Cache your DB-connection
Use SqlConnection.CreateCommand

Code Snippet Improvement for database call in .NET

4 Answers4