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

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 BindDataList(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();

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

        // 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;
}

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 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 - it's your method's job to add items to that list, not the client code's. Right?

• If you can return an IEnumerable, 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(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 BindDataList(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.

StackExchange Code Review Q#87807, answer score: 10