# Simple Dal for Accessing Procedures

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);
sda.Fill(dt);
return dt;
}

finally
{
DeIntialize();
}
}

SqlCommand GetParam(Dictionary<dynamic, dynamic> Params, SqlCommand cmd)
{
foreach (var objparam in Params)
{
}
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

}

• Welcome to CodeReview. Could you please add some textual context regarding your code and questions? – Kid Diamond Sep 11 '14 at 14:32
• We would like you to tell us what your code does, so we can see if something is wrong within it. – IEatBagels Sep 18 '14 at 14:18
• 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. – Patryk Ćwiek Sep 18 '14 at 15:00

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);
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())
{
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.

• Is spName Hungarian style? Presumably it's just a shortening of storedProcedureName – Ben Aaronson Sep 18 '14 at 14:20
• @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 – Malachi Sep 18 '14 at 14:24
• I would say spName is Hungarian done right. If I started concatenating it with other strings, it would look wrong. – RubberDuck Sep 18 '14 at 15:36
• @RubberDuck I agree with you, it makes perfect sense the way you put it. – Malachi Sep 18 '14 at 15:41