Optimize a generic foreach method that converts Datatable to my object using Reflection

Question

I need to optimize this code block below, this method converts a Datatable object to the Object that I am passing by parameter, in another words, this serializes the object, however I have to run this block 1 million times when reading data from the database, so this is the bottleneck of my application.

Obs: All my objects that I use in this method have a property "Id" which is the PrimaryKey of the table, but the name of the primary key in the database is different, so that's why I have that "if command" asking the name of the primary key.

How can I optimize this code below?

public static List<T> BindDataList<T>(DataTable dt, string primaryKey)
{
    // Get all columns' name
    var columns = (from DataColumn dc in dt.Columns select dc.ColumnName);

    var properties = typeof(T).GetProperties().ToList();

    var lst = new List<T>();

    foreach (DataRow dr in dt.Rows)
    {              
        // Create object
        var ob = Activator.CreateInstance<T>();

        // Get all properties
        properties.ForEach(propertyInfo =>
        {
            if (columns.Any(s => s.Equals(propertyInfo.Name, StringComparison.OrdinalIgnoreCase)))
            {
                // Fill the data into the property
                if (!(dr[propertyInfo.Name] is DBNull))
                    propertyInfo.SetValue(ob, dr[propertyInfo.Name]);
            }
            else
            {
                if (propertyInfo.Name.ToUpper().Equals("ID"))
                {
                    // Fill the data into the property
                    if (!(dr[primaryKey] is DBNull))
                    {
                        propertyInfo.SetValue(ob, dr[primaryKey].SafeToInt());
                    }
                }
            }
        });

        lst.Add(ob);
    }
    return lst;
}

Answer 1

A few points tickle:

• Why is it a static method?
• None of the comments are helpful. Good comments say why, not what. I'd remove them all.

• Bracing is inconsistent. Consider:

  if (!(dr[propertyInfo.Name] is DBNull))
      propertyInfo.SetValue(ob, dr[propertyInfo.Name]);
  

  And then:

  if (!(dr[primaryKey] is DBNull))
  {
      propertyInfo.SetValue(ob, dr[primaryKey].SafeToInt());
  }
  

  I prefer the latter, as explicit scopes are always less error-prone.

• Naming is also inconsistent. You call a PropertyInfo, propertyInfo (♥++)... but then you have a List<T> that you call lst instead of list - although since that's the method's return value I'd just call it result.

• Return value should probably be exposed as an IEnumerable<T> - it's your method's job to add items to that list, not the client code's. Right?
  • If you can return an IEnumerable<T>, then you can yield return ob, and drop the lst altogether.

• Instead of using reflection to create an instance of T, I would use a : new() generic type constraint to ensure T has a parameterless constructor, and create the instance like this instead:

  var ob = new T();
  

• There is no generic type constraint at all, so technically this call would be valid:

  var foo = WhateverTheClassIs.BindDataList<int>(dt, "Id");
  

  I would consider changing the signature to include a class generic type constraint that ensures T is a reference type:

  public static List<T> BindDataList<T>(DataTable dt, string primaryKey)
      where T : class, new()
  

• ob is an awful name. I understand the intent was to call it object and that would have clashed with System.Object - I would rather see @object than ob, to tell you the truth. But instance is probably a better fit.

• dr could be called row instead.

Answer 2

An optimization would be ...

Do the object property to database column matching up front, and only once.

Use that matching, to generate a list of functions which only essentially do the check for DBNull and the appropriate propertyInfo.SetValue.

For each row retrieved, create an instance of the object and invoke all the generated functions, passing it the database row and the created object.

Edit: As requested I have added an example to show intent. Note that this code is, not tested, it is just for reference, and is based on your original code.

    public static List<T> BindDataList<T>(DataTable dt, string primaryKey)
    {   
        // Get all columns' name
        var columns = (from DataColumn dc in dt.Columns select dc.ColumnName);

        var properties = typeof(T).GetProperties().ToList();

        var functions = 
            properties
            .Select(
                propertyInfo =>
                {
                    if (columns.Any(s => s.Equals(propertyInfo.Name, StringComparison.OrdinalIgnoreCase)))
                    {
                        return 
                            (ob, dr) =>
                            {
                                // Fill the data into the property
                                if (!(dr[propertyInfo.Name] is DBNull))
                                    propertyInfo.SetValue(ob, dr[propertyInfo.Name]);
                            };
                    }
                    else if (propertyInfo.Name.ToUpper().Equals("ID"))
                    {
                        return 
                            (ob, dr) =>
                            {
                                // Fill the data into the property
                                if (!(dr[primaryKey] is DBNull))
                                {
                                    propertyInfo.SetValue(ob, dr[primaryKey].SafeToInt());
                                }
                            };
                    }
                    else 
                        return (Action<T, DataRow>) null;
                })
                .Where(
                    function => function != null)
                .ToList();


            var lst = new List<T>();

            foreach (DataRow dr in dt.Rows)
            {              
                // Create object
                var ob = Activator.CreateInstance<T>();

                functions
                .ForEach(
                    f => f(ob, dr));

                lst.Add(ob);
            }

        return lst;
    }