2
\$\begingroup\$
 using System;
 using System.Collections.Generic;
 using System.Configuration;
 using System.Data;
 using System.Data.SqlClient;
 using System.Linq;
 using System.Web;

 namespace WebApplication1.App_Code
{
public class SqlHelper
{

    public string connstring;
    public SqlConnection con;
    public SqlCommand cmd;

    public SqlHelper(){}

    public SqlHelper(string sp)
    {
        connstring = ConfigurationManager.AppSettings["constr"];
        con = new SqlConnection(connstring);
        cmd = new SqlCommand(sp, con);
        cmd.CommandType = CommandType.StoredProcedure;
    }

}

public class Result : SqlHelper
{
    public Result(string sp) : base(sp)
    {
    }

    public bool ExecuteResultWithParameter(Dictionary<string, string> parameter)
    {

        bool result = true;
        try
        {             
            foreach (var item in parameter)
            {
              cmd.Parameters.AddWithValue("@" + item.Key, item.Value);
            }
            con.Open();
            cmd.ExecuteNonQuery();
        }
        catch (Exception Ex)
        {
            result = false;
        }
        finally
        {
            cmd.Dispose();
            con.Close();
            con.Dispose();
        }
        return result;
    }


    public DataSet ReturnResultWithParameter(string sp, Dictionary<string, string> parameter)
    {

        DataSet dt = new DataSet();
        try
        {
      
         foreach (var item in parameter)
        {
            cmd.Parameters.AddWithValue("@" + item.Key, item.Value);
        }
        using (SqlDataAdapter sda = new SqlDataAdapter(cmd))
        {
            sda.Fill(dt);
        }

        }
        catch (Exception Ex)
        {

        }
        finally
        {
            cmd.Dispose();
            con.Close();
            con.Dispose();
        }
        return dt;
    }

    public DataSet ReturnResultWithoutParameter()
    {
        DataSet dt = new DataSet();
        try
        {
           
            using (SqlDataAdapter sda = new SqlDataAdapter(cmd))
            {

                sda.Fill(dt);
            }           
        }
        catch (Exception Ex)
        {

        }
        finally
        {
            cmd.Dispose();
            con.Close();
            con.Dispose();
        }


        return dt;
    }


}

 }

I have written SQL helper class for my project and I want to improve the above code.also can someone suggest a suitable design pattern for the same.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Use Dapper instead of writing your own ORM. \$\endgroup\$
    – BCdotWEB
    Commented Sep 27, 2020 at 18:38

2 Answers 2

3
\$\begingroup\$

Don't use public fields. Instead, make them public properties.

Use proper naming.
What is sp? storedProcedureName will be better.

Dictionary is a collection of parameters. Therefore, use the plural name: parameters.

Using the AddWithValue method may result in poor query performance. Use the Add method with the exact SqlDbType type specified.

The ExecuteNonQuery method returns the number of rows affected. Therefore, your ExecuteResultWithParameter method should return this value.
You can get rid of the bool result variable. The failure of the method should be signaled by an exception that is re-thrown in the catch block: throw ex; (please note: the name starts with a small letter).

The ReturnResultWithParameter method has an unused sp parameter. Remove it.

You swallow exceptions with empty catch blocks. Therefore, there is no way to find out about query errors.

In all methods, you dispose of the connection and the command in the finally blocks. After that, the methods cannot be called again without re-instantiating the class.

There are extra empty lines in your code - this is also an error.

Many of the errors I described will catch Visual Studio extensions like FxCop, StyleCop, and others. They will not only find them, but also suggest ways to fix them. Start using these extensions.

\$\endgroup\$
1
\$\begingroup\$

Try to abstract away from helper name, when class is properly named - the whole picture become clear.

Low level helper logic shouldn't be visible, instead we just need such usage

using (var connection = GetConnection())
{
    connection.Execute("procedureName", new {A = "A", B = "B" });
    connection.CommitChanges();
}

Parameters passing is the feature of language,

 public abstract void Execute(string sp, params object[] parameters);

Execute method will use converter inside, like

    public IDictionary<string, object> GetObjectValues(object obj)
    {
        IDictionary<string, object> result = new Dictionary<string, object>();
        if (obj == null)
        {
            return result;
        }

        foreach (var propertyInfo in obj.GetType().GetProperties())
        {
            string name = propertyInfo.Name;
            object value = propertyInfo.GetValue(obj, null);
            result[name] = value;
        }

        return result;
    }

Inherit from IDisposable, move the contents of finally blocks into Dispose() method implementation.

Check Dapper.

https://solrevdev.com/2011/04/10/dapper-dot-net-simple-sql-object-mapper.html

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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