3
\$\begingroup\$

Looking for feedback for a class I'm writing to manage my db connections/queries on my site (.net core c# web api, MS Sql DB). The reason behind the class was the ever growing list of db calls and wanting to centralize not only the connection and query, but to also have it populate by objects through a series of methods using reflection. I am only using a single database.

public class DbQuery : IDisposable
{
    private SqlConnection con;
    private SqlCommand com;
    private SqlTransaction trans;
    private List<SqlParameter> parameters;

    public DbQuery(string connectionString)
    {
        con = new SqlConnection(connectionString);            
        OpenConnection();
    }

    public void BeginTransaction()
    {
        trans = this.con.BeginTransaction();
    }

    public void BeginTransaction(IsolationLevel isolationLevel)
    {
        trans = this.con.BeginTransaction(isolationLevel);
    }

    public void CommitTransaction()
    {
        trans.Commit();
    }

    public void RollbackTransaction()
    {
        trans.Rollback();
    }

    public void ClearParameters()
    {
        parameters.Clear();
    }

    public void AddParameter(string key, dynamic value)
    {
        if (parameters == null)
        {
            parameters = new List<SqlParameter>();
        }
        parameters.Add(new SqlParameter(key, value));
    }

    public T ExecuteReturnObject<T>(string sSql, CommandType commandType = CommandType.Text) where T : new()
    {
        try
        {
            return ExecuteSql<T>(sSql, commandType);
        }
        catch (SqlException e)
        {
            throw;
        }
        catch (Exception e)
        {
            throw;
        }
    }

    public List<T> ExecuteReturnList<T>(string sSql, CommandType commandType = CommandType.Text) where T : new()
    {
        try
        {
            return ExecuteSqlList<T>(sSql, commandType);
        }
        catch (SqlException e)
        {
            throw;
        }
        catch (Exception e)
        {
            throw;
        }
    }

    public T ExecuteReturnScalar<T>(string sSql, CommandType commandType = CommandType.Text)
    {
        try
        {
            return ExecuteSqlScalar<T>(sSql, commandType);
        }
        catch (SqlException e)
        {
            throw;
        }
        catch (Exception e)
        {
            throw;
        }
    }

    public int ExcuteNonQuery(string sSql, CommandType commandType = CommandType.Text)
    {
        try
        {
            return ExcuteSqlNonQuery(sSql, commandType);
        }
        catch(SqlException)
        {
            throw;
        }
        catch(Exception)
        {
            throw;
        }
    }

    private List<T> ExecuteSqlList<T>(string sSql, CommandType commandType) where T : new()
    {
        try
        {
            CreateCommand();
            com.CommandText = sSql;
            com.CommandType = commandType;

            if (parameters != null && parameters.Any())
            {
                com.Parameters.AddRange(parameters.ToArray());
            }

            using (SqlDataReader reader = com.ExecuteReader())
            {
                List<T> tList = new List<T>();
                while (reader.Read())
                {
                    T t = new T();
                    for (int i = 0; i < reader.FieldCount; i++)
                    {
                        string field = reader.GetName(i);
                        TrySetProperty(t, field, reader.GetValue(i));
                    }
                    tList.Add(t);
                }
                return tList;
            }
        }
        catch (SqlException e)
        {
            throw;
        }
        catch (Exception)
        {
            throw;
        }
        finally
        {
            DisposeCommand();
        }
    }

    private T ExecuteSql<T>(string sSql, CommandType commandType) where T : new()
    {
        try
        {
            CreateCommand();
            com.CommandText = sSql;
            com.CommandType = commandType;
            if (parameters != null && parameters.Any())
            {
                com.Parameters.AddRange(parameters.ToArray());
            }

            using (SqlDataReader reader = com.ExecuteReader())
            {
                T t = new T();
                reader.Read();
                for (int i = 0; i < reader.FieldCount; i++)
                {
                    string field = reader.GetName(i);                        
                    TrySetProperty(t, field, reader.GetValue(i));
                }
                return t;
            }
        }
        catch (SqlException e)
        {
            throw;
        }
        catch (Exception)
        {
            throw;
        }
        finally
        {
            DisposeCommand();
        }
    }

    private int ExcuteSqlNonQuery(string sSql, CommandType commandType)
    {
        try
        {
            CreateCommand();
            com.CommandText = sSql;
            com.CommandType = commandType;
            if (parameters != null && parameters.Any())
            {
                com.Parameters.AddRange(parameters.ToArray());
            }
            int nRowAffected = com.ExecuteNonQuery();
            return nRowAffected;
        }
        catch (SqlException e)
        {
            throw;
        }
        catch (Exception)
        {
            throw;
        }
        finally
        {
            DisposeCommand();
        }
    }

    private T ExecuteSqlScalar<T>(string sSql, CommandType commandType) 
    {
        try
        {
            CreateCommand();
            if(parameters != null && parameters.Any())
            {
                com.Parameters.AddRange(parameters.ToArray());
            }

            com.CommandText = sSql;
            com.CommandType = commandType;
            T t = (T)com.ExecuteScalar();

            return t;
        }
        catch (SqlException)
        {
            throw;
        }
        catch(Exception)
        {
            throw;
        }
        finally
        {
            DisposeCommand();
        }
    }

    private void TrySetProperty(object obj, string property, object value)
    {
        if(value == DBNull.Value || value == null)
        {
            return;
        }

        var prop = obj.GetType().GetProperty(property, BindingFlags.Public | BindingFlags.Instance | BindingFlags.IgnoreCase);
        if (prop != null && prop.CanWrite)
            prop.SetValue(obj, value, null);
    }

    private void OpenConnection()
    {
        if (con.State == ConnectionState.Closed)
        {
            con.Open();
        }
    }

    private void CloseConnnection()
    {
        if (con.State != ConnectionState.Closed)
        {
            Console.WriteLine("closing connection");
            con.Close();
        }
    }

    private void CreateCommand()
    {
        com = this.con.CreateCommand();
        if (trans != null)
        {
            com.Transaction = trans;
        }
    }

    private void DisposeCommand()
    {
        if (com != null)
        {
            Console.WriteLine("disposing command");
            com.Dispose();
        }
    }

    #region IDisposable Support
    private bool disposedValue = false; // To detect redundant calls

    protected virtual void Dispose(bool disposing)
    {
        if (!disposedValue)
        {
            if (disposing)
            {
                // TODO: dispose managed state (managed objects).
            }

            // TODO: free unmanaged resources (unmanaged objects) and override a finalizer below.
            // TODO: set large fields to null.

            CloseConnnection();

            if (con != null)
            {
                Console.WriteLine("disposing connection");
                con.Dispose();
            }
            disposedValue = true;
        }
    }

    // TODO: override a finalizer only if Dispose(bool disposing) above has code to free unmanaged resources.
    ~DbQuery()
    {
        // Do not change this code. Put cleanup code in Dispose(bool disposing) above.
        Dispose(false);
    }

    // This code added to correctly implement the disposable pattern.
    public void Dispose()
    {
        // Do not change this code. Put cleanup code in Dispose(bool disposing) above.
        Dispose(true);
        // TODO: uncomment the following line if the finalizer is overridden above.
        GC.SuppressFinalize(this);
    }
    #endregion
}

the use of it is as such

using (DbQuery db = new DbQuery(ConnecitonString))
{
    string sql = "select somthing from somewhere where id=@id";
    db.AddParameter("id", 2);
    string title = db.ExecuteReturnScalar<string>(sql);
}

still in the design and testing phase, but as mentioned, looking for some feedback, good or bad.

\$\endgroup\$
  • 1
    \$\begingroup\$ soo ... you're reinventing Prepared Statements? \$\endgroup\$ – Vogel612 Aug 13 '17 at 17:58
  • 1
    \$\begingroup\$ complete code is posted. the todos were generated from the IDisposable implementation. \$\endgroup\$ – spiral Aug 13 '17 at 19:28
  • \$\begingroup\$ Why use raw ado instead of an ORM? \$\endgroup\$ – RubberDuck Aug 13 '17 at 21:11
1
\$\begingroup\$
public T ExecuteReturnObject<T>(..)
{
    try
    {
        return ExecuteSql<T>(sSql, commandType);
    }
    catch (SqlException e)
    {
        throw;
    }
    catch (Exception e)
    {
        throw;
    }
}

Without any exeption handling like logging etc this and that

public T ExecuteReturnObject<T>(..)
{
    return ExecuteSql<T>(sSql, commandType);
}

are exactly the same. Catching and rethrowing exceptions without handling them is pointless.


private SqlCommand com;

private void CreateCommand()
{
    com = this.con.CreateCommand();
    if (trans != null)
    {
        com.Transaction = trans;
    }
}

You are assigning the command to a field. This is a very bad idea. If you call the Execute methods in a multithreaded scenario each thread will overwrite the work of the other one(s). It's better here to use function local commands rather then a class wide field. There also a risk that each thread will dispose the other one(s) command as you also call the DisposeCommand method.

Have you considered to create IDbConnection extensions instead? You may also take a look at Dapper? It does exactly the same thing as your class. Executing queries and mapping them to objects.

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

1. Connection String Create a DbQuery with the Parameterless constructor and read the connection string from web.config.

    public DbQuery()
    {
        con = new SqlConnection(ConfigurationManager.ConnectionStrings["AppContext_ConnectionString_Name"].ConnectionString);
        OpenConnection();
    }

Sample code to call the query,

using (DbQuery db = new DbQuery())
    {
            string sql = "select somthing from somewhere where id=@id";
            db.AddParameter("id", 2);
            string title = db.ExecuteReturnScalar<string>(sql);
    }   

This way, No need to pass the connection string as a parameter to all the DbQuery Constructor.

Maintenance wise, if we need to change the connection string related code, then change is at one place instead of multiple places.

2. SqlParameters: The below logic applies to all the methods depends on the Sql Parameters. Here, I am taking the example of ExecuteReturnScalar method now.

Updated method logic:

    public T ExecuteSqlTextScalar<T>(string sSql, params SqlParameter[] parameters)
    {
        return ExecuteSqlScalar<T>(sSql, CommandType.Text, parameters);
    }

    public T ExecuteReturnScalar<T>(string sSql, CommandType commandType, params SqlParameter[] parameters)
    {
        try
        {
            return ExecuteSqlScalar<T>(sSql, commandType, parameters);
        }
        catch (SqlException e)
        {
            throw;
        }
        catch (Exception e)
        {
            throw;
        }
    }

Now call the method like below

    using (DbQuery db = new DbQuery())
        {
            string sql = "select somthing from somewhere where id=@id";
            string title = db.ExecuteSqlTextScalar<string>(sql,
                new SqlParameter("id", 2),
                new SqlParameter("Name", "Test1"),
                new SqlParameter("Address", "Address1"));
        }

Advantages:

. Add any number of SqlParameters with comma separated list

. Allows to call the methods parallelly and supports async calls too. As method calls are not depending on the SqlParameter member variable.

. Can delete the below properties and methods from the code

- Parameters member variable 

- ClearParameter and AddParameter methods.

. Microsoft Entity framework also follows the same logic. Please check the below link. https://msdn.microsoft.com/en-us/library/system.data.entity.database.executesqlcommand(v=vs.113).aspx#M:System.Data.Entity.Database.ExecuteSqlCommand(System.String,System.Object[])?

3. Exception Handling:

Right now, Exception handling applied for both Private and Public methods.

    public int ExcuteNonQuery(string sSql, CommandType commandType = CommandType.Text)
    {
        try
        {
            return ExcuteSqlNonQuery(sSql, commandType);
        }
        catch (SqlException)
        {
            throw;
        }
        catch (Exception)
        {
            throw;
        }
    }

    private int ExcuteSqlNonQuery(string sSql, CommandType commandType, params SqlParameter[] parameters)
    {
        try
        {

        }
        catch (SqlException e)
        {
            throw;
        }
        catch (Exception)
        {
            throw;
        }
        finally
        {
            DisposeCommand();
        }
    }

In the above case,

• Remove the exception handling in public method

• If there is no additional logic in Public method(Apart from calling the Private method) then Move the logic from Private method to Public method and Delete Private method.

\$\endgroup\$

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.