2
\$\begingroup\$

The following helper method executes commands asynchronously and returns a Task<IEnumerable<T>> where T is a type that receives the row information from the SqlDataReader rows. All works fine.

    public static async Task<IEnumerable<T>> CommandExecuteReaderAsync<T>(SqlConnection conn, SqlTransaction trans, CommandType commandType, string commandText, IEnumerable<SqlParameter> parameters, CancellationToken token, int commandTimout)
    {
        using (var command = new SqlCommand(commandText, conn, trans))
        {
            command.CommandTimeout = commandTimout;
            command.Parameters.AddRange(parameters.ToArray());
            command.CommandType = commandType;
            using (var sr = await command.ExecuteReaderAsync(token))
            {
                var dc = new System.Data.Linq.DataContext(conn);

                return dc.Translate<T>(sr).ToList();
            }
        }
    }

However, I'm not happy with my current approach of creating a new DataContext and using it's Translate<T> method.

Is there a better approach as the creation of the DataContext seems a bit excessive?

\$\endgroup\$
  • 1
    \$\begingroup\$ Please explain what this DataContext is? Ideally add it to the question. \$\endgroup\$ – t3chb0t Nov 26 '17 at 14:10
  • 1
    \$\begingroup\$ It seems to be a Linq2Sql DataContext. \$\endgroup\$ – Adrian Iftode Nov 26 '17 at 14:23
  • \$\begingroup\$ @t3chb0t - Apologies, I've edited the original post. Its the System.Data.Linq.DataContext type. \$\endgroup\$ – Cleve Nov 26 '17 at 15:09
4
\$\begingroup\$

DataContext

[..]the creation of the DataContext seems a bit excessive

Why do you think so? Is creating the SqlCommand not excessive? You are instantiating it the same number of times.

If DataContext.Translate helps you to save time by not writing the mapping your self then I'd say it's fine. Just remember that DataContext is a disposable type too, so you should add one more using for it.


Naming

Apart from the DataContext you should try to make you code more consistent. This means, don't use abbreviations like conn or trans. Use full words especially for parameters.


Style

public static async Task<IEnumerable<T>> CommandExecuteReaderAsync<T>(
  SqlConnection conn, 
  SqlTransaction trans, 
  CommandType commandType, 
  string commandText, 
  IEnumerable<SqlParameter> parameters, 
  CancellationToken token, 
  int commandTimout)

I hope you have some overloads for this method. 7 (!) parameters, this is a lot. You should try to reduce them and in fact you can hide at least three.

If you make it an extension for the SqlConnection then you'll have one parameter less that you have to specify.

If you use TransactionScope then you can get rid of the second parameter, the SqlTransaction.

If you write two more helper methods (ExecuteQueryAsync & ExecuteStoredProcedureAsync) that call this method, then you can get rid of the CommandType parameter.

So no you just have four parameters left that you must specify: commandText, cancelltionToken & commandTimeout & parameters.

\$\endgroup\$
  • \$\begingroup\$ thanks for your feedback. I did mark up your answer but my low rating doesn't add to the count! Thank again. \$\endgroup\$ – Cleve Nov 27 '17 at 7:47
1
\$\begingroup\$

I'm not happy with your current approach of creating a new DataContext and using it's Translate<T> method either. The task at hand, ostensibly, is convert an IDataReader to a generic type T.. much like an O/RM would do. Mixing raw ADO.NET and O/RM technologies seems odd. Why not just use Entity Framework, nHibernate, or Dapper? The code to do what you're wanting is pretty complex - that's why whole products exist to do just this. A simple path (which I would not recommend, for performance and corner-case reasons) would go something like this:

List<T> resultSet = new List<T>();

while (sr.Read())
{
    T row = new T();

    for (int i = 0; i < sr.FieldCount; i++)
    {
        if (sr.GetValue(i) != DBNull.Value)
        {
            PropertyInfo propertyInfo = typeof(T).GetProperty(sr.GetName(i));

            propertyInfo.SetValue(row, sr.GetValue(i), null);
        }
    }

    resultSet.Add(row);
}

return resultSet;
\$\endgroup\$
  • \$\begingroup\$ Hi, thanks for your feedback. Much appreciated. \$\endgroup\$ – Cleve Nov 27 '17 at 7:48

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.