6
\$\begingroup\$

I've written a very light wrapper around Dapper to help with unit testing and streamlining the Dapper commands I need.

Execute() is a method created to remove the code duplication of opening and closing the database connection and enlisting it in a transaction. The code is very quick at inserts with this method (200,000 inserts in less than 30 secs).

I'm wondering if I've made any obvious mistakes with regards to Dapper and performance.

For instance, should a single insert or read explicitly enlist a transaction, or would they be faster with an ambient transaction?

public class DappperService 
{
    private readonly string sqlConnection;

    public DappperService(string sqlConnection)
    {
        this.sqlConnection = sqlConnection;
        DapperExtensions.DapperExtensions.DefaultMapper = typeof(PluralizedAutoClassMapper<>);
    }

    public T Get<T>(int id) where T : class
    {
        return Execute((x, t) => x.Get<T>(id, t));
    }

    public bool Delete<T>(T dto) where T : class
    {
        return Execute((x, t) => x.Delete(dto,t));
    }

    public dynamic Insert<T>(T dto) where T : class
    {
        return Execute((x, t) => x.Insert(dto,t));
    }

    public void Insert<T>(List<T> dtoList) where T : class
    {
        IEnumerable<T> enu = dtoList;

        using (var con = new SqlConnection(sqlConnection))
        {
            con.Open();
            var trans = con.BeginTransaction();
            con.Insert(enu, trans);
            trans.Commit();
            con.Close();
        }
    }

    public IEnumerable<T> GetList<T>() where T : class
    {
        return Execute((x, t) => x.GetList<T>(transaction:t));
    }

    public dynamic Execute(Func<SqlConnection, SqlTransaction, dynamic> action)
    {
        using (var con = new SqlConnection(sqlConnection))
        {
            con.Open();
            var trans = con.BeginTransaction();
            var result = action(con, trans);
            trans.Commit();
            con.Close();
            return result;
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ I think you are going about this in the wrong way. In your case you have created a class for an object that wraps SQLconnection with basic Get, Delete, Insert obj, insert obj[] from an architecture and design stand-point database connections should be made within your Repos not services. ( debating this is a waste of time ) When you have SomethingRepo you can inherit a Repo that contains your database wrapper. \$\endgroup\$ – Plixxer May 15 at 16:22
  • \$\begingroup\$ Also your execute is using a dynamic which can use an anonymous type instead. I only really like to use dynamic when i have no way of knowing what a type is but still need to output it to a restful service. \$\endgroup\$ – Plixxer May 15 at 16:23
4
\$\begingroup\$

Naming

I recommend not using acronyms or shortened names where possible for your variables. You might as well use the full name, it aids in quick reading.

e.g.

public dynamic Insert<T>(T dataTransferObject) where T : class
{
    return Execute((x, t) => x.Insert(dataTransferObject,t));
}

Although for that method item may work just as well.

The same goes for single-letter variable names in your lambda expressions. If there's only parameter to your lambda, it's probably ok, but with multiple parameters it can quickly become confusing.

return Execute((connection, transaction) => connection.Insert(dataTransferObject,transaction));

Shows the exact purpose of your lambda much more clearly.

I also note that you have a method called GetList that does not return a List. This is just plain confusing.

Lastly, you should avoid putting the type of the variable in its name.

E.g.

public void Insert<T>(List<T> dtoList)

would be better as:

public void Insert<T>(List<T> dtos)

Otherwise it can lead to confusion should you decide to change it to a different type, and forget to update the variable name.

var

You have used var appropriately and consistently, making this (as far as I'm aware) the first code review I've ever posted that does not contain some guidance on var. Well done.

Parameter types

You take a List with this method:

public void Insert<T>(List<T> dtoList) where T : class
{
    IEnumerable<T> enu = dtoList;
    // yadda yadda yadda...

And then immediately convert it to an IEnumerable in your body. Just take the parameter as IEnumerable in the first place, passing a List in will still work. By requiring a List you're actually making it harder to use your own function (because you can't, for example, pass in an array) for no reason at all.

\$\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.