Simple Dal for Accessing Procedures

Question

How could I improve this code?

public class DALUser
 {
 SqlConnection conn;
 SqlCommand cmd;
 public void ExecuteSp(string Spname, Dictionary<dynamic, dynamic> Params)
 {
 try
 {
 initialize();
 cmd = new SqlCommand();
 cmd.Connection = conn;
 cmd.CommandText = Spname;
 cmd.CommandType = CommandType.StoredProcedure;
 cmd = GetParam(Params, cmd);
 cmd.ExecuteNonQuery();
 }
 finally
 {
 DeIntialize();
 }
 }
 public DataTable ExecuteAdapter(string Spname, Dictionary<dynamic, dynamic> Params)
 {
 try
 {
 initialize();
 cmd = new SqlCommand();
 cmd.Connection = conn;
 DataTable dt = new DataTable();
 cmd.CommandText = Spname;
 cmd.CommandType = CommandType.StoredProcedure;
 cmd = GetParam(Params, cmd);
 SqlDataAdapter sda = new SqlDataAdapter(cmd);
 sda.Fill(dt);
 return dt;
 }
 finally
 {
 DeIntialize();
 }
 }
 SqlCommand GetParam(Dictionary<dynamic, dynamic> Params, SqlCommand cmd)
 {
 foreach (var objparam in Params)
 {
 cmd.Parameters.AddWithValue(Const.sParamPrefix + objparam.Key, objparam.Value);
 }
 return cmd;
 }
 #region SqlConnection Side
 void initialize()
 {
 if (CheckConnection())
 {
 conn = new SqlConnection(Const.ConnectionString);
 conn.Open();
 }
 }
 bool CheckConnection()
 {
 if (conn == null)
 return true;
 return false;
 }
 void DeIntialize()
 {
 if (!CheckConnection())
 {
 conn.Close();
 conn.Dispose();
 }
 }
 #endregion
 }

• Kid Diamond: Welcome to CodeReview. Could you please add some textual context regarding your code and questions?
• IEatBagels: We would like you to tell us what your code does, so we can see if something is wrong within it.
• Patryk Ćwiek: It's completely beside the point of the review, but if you're working with SP-heavy database, maybe you should look into one of the micro-ORMs that makes it very easy to call stored procedures, for example Insight.Database, instead of rolling out your own solution.

Answer 1

Your initialize and DeInitialize do the job of connecting and disconnecting your SQL Connection, but not the SQL Command that you create in each method.

I am sure that closing and disposing the connection will take care of the Command but I like to make sure.

so what I would do is to use a using statement

initialize();
using (SqlCommand cmd = new SqlCommand() { 
 .Connection = conn,
 .CommandText = Spname,
 .CommandType = CommandType.StoredProcedure
})
{
 cmd = GetParam(Params, cmd);
 cmd.ExecuteNonQuery();
}

your try/catch works, but I would rather have my connection opened and closed inside each method and not have to call 2 other methods and a try/catch to make it work. I would much rather just do this

using (SqlConnection conn = new SqlConnection(Const.ConnectionString))
using (SqlCommand cmd = new SqlCommand() { 
 .Connection = conn,
 .CommandText = Spname,
 .CommandType = CommandType.StoredProcedure
})
{
 cmd = GetParam(Params, cmd);
 cmd.ExecuteNonQuery();
}

and that gets rid of the try catch and all the ugly close/dispose calls.

It also gets rid of the need for a check connection method because when you use a using statement there is no open connection outside of the using block.

---

And OCD tells me to change this as well

public DataTable ExecuteAdapter(string Spname, Dictionary<dynamic, dynamic> Params)
{
 try
 {
 initialize();
 cmd = new SqlCommand();
 cmd.Connection = conn;
 DataTable dt = new DataTable();
 cmd.CommandText = Spname;
 cmd.CommandType = CommandType.StoredProcedure;
 cmd = GetParam(Params, cmd);
 SqlDataAdapter sda = new SqlDataAdapter(cmd);
 sda.Fill(dt);
 return dt;
 }
 finally
 {
 DeIntialize();
 }
}

and make it look like this instead

public DataTable ExecuteAdapter(string Spname, Dictionary<dynamic, dynamic> Params)
{
 using (SqlCommand cmd = New SqlCommand(){
 .Connection = conn,
 .CommandText = Spname,
 .CommandType = CommandType.StoredProcedure
 })
 {
 cmd = GetParam(Params,cmd);
 using (DataTable dt = new DataTable())
 using (SqlDataAdapter sda = new SqlDataAdapter(cmd))
 {
 sda.Fill(dt);
 return dt;
 }
 }
}

---

Naming

initialize();

should be capitalized for standards sake

Initialize();

Method names are PascalCase
variables are camelCase

I know this probably isn't the real SPROC name but Spname should be spName even though Hungarian name scheme is no longer a recommended name scheme.

• Ben Aaronson: Is spName Hungarian style? Presumably it's just a shortening of storedProcedureName
• Malachi: @BenAaronson, I always thought that Hungarian was type abbreviation and then a name for the variable like strName, and now I see your point, because Stored Procedure isn't the type of that variable it's actually a string. something to think about for sure
• RubberDuck: I would say spName is Hungarian done right. If I started concatenating it with other strings, it would look wrong.
• Malachi: @RubberDuck I agree with you, it makes perfect sense the way you put it.