0
\$\begingroup\$

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


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

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.

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

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.