Trying to write better code 2

[fergmj]

I am new to C# and programming in .NET

I am pretty sure this needs some better error handling around the connection at least.

Can anyone suggest improvements to my small code below?

Thank you in advance.

                SQLErrors = "";
                SqlConnection cn = new SqlConnection(connectionStringGP);
                cn.Open();

                string sql = "exec tdbTransferWarehouseItemGetDocNo '" + sEmpInitials + "'";

                SqlCommand cd = new SqlCommand(sql, cn);

                DocNo = "";
                bDupDocNo = false;

                SqlDataReader dr2 = cd.ExecuteReader();

                if (dr2.Read())
                {

                    DocNo = dr2["DocNo"].ToString();

                }
                dr2.Close();

 

[macleod1021]

The quick answer is to use a try catch block around the code.

Whenever you write code that could cause an error, you should wrap it within a try catch block. That way, you can dictate what happens when an error occurs instead of the OS.

im in ur stakz, overflowin ur heapz!

 

[JurkMonkey]

try, catch, finally....

The finally should be used to dispose of your connection properly...

http://www.ubuntu.com/

 

[Craig0201]

The quick answer is to use a try catch block around the code.

Whenever you write code that could cause an error, you should wrap it within a try catch block. That way, you can dictate what happens when an error occurs instead of the OS."

I disagree. You should wrap called code if you know how to fix the problem. If not, you should wrap the caller code.

In this case, with a finally block to clear up the connection, you may wish to rethrow the exception.

C

 

[phinoppix]

In addition, but not related to exception handling, it seems that in your sp, you're only expecting a single value. You can use the SqlCommand.ExecuteScalar() method. It will return the first column of the first row only. If resultset is empty, a null reference is returned.

Also, use command parameters, rather than concatenating. Keeps you safe from sql injection.

hth

 

[Smeat]

I agree with Craig0201, you should only catch exceptions if you are going to do something about them, else let the exception bubble up.

Smeat

 

[BlackburnKL]

If you are returning a single value to load into a variable, for example, returning a parameter avoids all of the overhead of a data table that you would get even with ExecuteScalar.

BlackburnKL

 

[jmeckley]

always use a parameterized query. this avoids sql injection attacks. here's a simpilified version of how I code connections.

using(IDbConnection cnn = new SqlConnection(connectionStringGP))
{
   cnn.Open();
   IDbCommand cmd = cnn.CreateCommand();
   cmd.CommandText = "tdbTransferWarehouseItemGetDocNo @input";
   cmd.CommandType = CommandType.StoredProcecure;
   IDbParameter parameter = cmd.CreateParameter("input");
   parameter.Value = sEmpInitials;
   cmd.Parameters.Add(parameter);
   return cmd.ExecuteScalar();
}

Jason Meckley
Programmer
Specialty Bakers, Inc.

 

[NeilTrain]

always use a parameterized query. this avoids sql injection attacks.

More than that, they also are faster in the DB. SQL will cache the statement, but not the parameters. So the next time the same statement is used it is already compiled and has an execution plan, SQL server just plugs in the params. Using prepared sql statements with inline parameters clogs up the cache, unless you expect the same params every time. But even then, use parameterazed for security's sake.

 

[mmilan]

Consider implementing your stored procedure as a user defined function instead...

The advice about parameters is bang on - follow it...

Martin.

 

[JoeAtWork]

Consider implementing your stored procedure as a user defined function instead...

I don't agree with that. User defined functions should be used inline with SQL statements. They are meant for doing some extra manipulation of the data before returning it. For simply selecting data, a stored procedure should be used.

 

[chiph]

But you can use a table-valued function to compose up larger result sets from smaller ones. This is a pretty powerful instance of reuse in the database layer.

Chip H.


____________________________________________________________________
www.chipholland.com