4
\$\begingroup\$

I have an extension that adds values to a class and I was wondering if there was a faster way to do this.

Does a for loop execute faster or would a LINQ query do better?

public static List<T> DataTableToList<T>(this DataTable dt) where T : class, new()
{
    List<T> dataTableList = new List<T>();
    var rowsCount = dt.Rows.Count;
    var columnCount = dt.Columns.Count;

    for (int i = 0; i < rowsCount; i++)
    {
        T obj = new T();
        int j = -1;
        Type type = obj.GetType();
        PropertyInfo[] properties = type.GetProperties();

        foreach (PropertyInfo property in properties)
        {
            if (j == -1)
            {
                property.SetValue(obj, Guid.NewGuid());
            }
            else if (j < columnCount)
            {
                if (dt.Rows[i][j] == null) continue;
                property.SetValue(obj, dt.Rows[i][j].ToString());
            }
            j++;
        }
        dataTableList.Add(obj);
    }
    return dataTableList;
}
\$\endgroup\$
  • \$\begingroup\$ Why do you need this to be faster? Is this slow? How did you measure it? \$\endgroup\$ – t3chb0t Nov 10 '18 at 15:56
  • \$\begingroup\$ @t3chb0t, I have not measured the speed, but I was wondering if there was a faster way of doing this? \$\endgroup\$ – KyloRen Nov 11 '18 at 13:06
  • \$\begingroup\$ Why not use Dapper -- github.com/StackExchange/Dapper -- instead of writing ADO.NET code? \$\endgroup\$ – BCdotWEB Nov 12 '18 at 11:13
7
\$\begingroup\$

I wouldn't say, your major concern should be performance. I find the method rather error prone and it has a very limited use:

1) All columns have to be of type string - except the first which must be of type Guid

2) The order of the columns in the table must match the order of the properties of the object type. If they don't, a column could easily be mapped with a wrong property without no warning because they must all be strings. You definitely need some type checking.

3) A data table usually has a (unique) Id column, which the instances of the class should be instantiated with, but it seems, that you create a new object Id for every new instance/row. Is that Id saved to the database? And what about the next time the Row/Item are mapped - is a new Id again created for that item? It seems not logical to have a new id for an item every time it is fetched from a database?

I would say that the method is so aimed at a special situation where you need to map a data table with a certain class that it is not a candidate as an extension method - but just as a private method in a class. An extension method should be generic as much as possible and should handle all possible errors in a consistent way.

What about private vs. public members?


I personally don't like the use of an indexed iteration here: I would go along these lines:

  PropertyInfo[] pis = typeof(T).GetProperties();

  foreach (DataRow row in dt.Rows)
  {
    T item = new T();

    foreach (DataColumn column in dt.Columns)
    {
      PropertyInfo pi = pis.FirstOrDefault(p => p.Name == column.ColumnName);
      if (pi != null)
      {
        pi.SetValue(item, row[column]);
      }
    }
  }

or

  PropertyInfo[] pis = typeof(T).GetProperties();

  foreach (DataRow row in dt.Rows)
  {
    T item = new T();

    foreach (PropertyInfo pi in pis)        
    {
        pi.SetValue(item, row[dt.Columns[pi.Name]]);
    }
  }

Notice, that the call to typeof(T).GetProperties() is outside the loops, because there is no need to call it for every row/item. In this way you are half the way to a nearly completely generic extension method.


In order to make it more generic, you'll have to consider the mapping strategy:

1) Should the column names match the property names of the class or should you consider to be able to provide a mapping table?:

public static List<T> DataTableToList<T>(this DataTable dt, Dictionary<string, string> mappings) where T : class, new()

or:

public static List<T> DataTableToList<T>(this DataTable dt, (string column, string property)[] mappings) where T : class, new()

You'll also have have to consider conversion of certain types: for instance is DateTime often stored in a data table as SqlDateTime which can not implicitly cast to a DateTime structure, so you should maybe be able to provide some converters:

public static List<T> DataTableToList<T>(this DataTable dt, (string column, string property)[] mappings, Dictionary<string, Func<object, object>> converters) where T : class, new()

called like:

    Dictionary<string, Func<object, object>> converters = new Dictionary<string, Func<object, object>>
    {
      { "BirthDay", (sqlDate) => ((SqlDateTime)sqlDate).Value }
    };


    List<MyItem> items = table.DataTableToList<MyItem>(null, converters);

Both the mapping and converter strategy can probably be more elegant or sophisticated - the above is merely some quick suggestions.

\$\endgroup\$
  • 1
    \$\begingroup\$ +1, but that FirstOrDefault call in the innermost loop is repeatedly doing the same work. Better to create an array of valid (column, propertyInfo) tuples up-front and use that in the inner loop. \$\endgroup\$ – Pieter Witvoet Nov 12 '18 at 8:00

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.