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


    }

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.