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

• Mathieu Guindon: Welcome to CR! I hope you get great reviews!
• Roger: Thanks @Mat'sMug! I've been using stackoverflow.com now I found this community, which looks also great!
• Dan Lyons: Where is the DataTable coming from? If it's coming from a database as the question implies, you might be far better off pulling the data into your data objects directly, either through an IDataReader or even better, an ORM.
• Roger: but is it faster than the adapter.fill()?
• Zer0: I'm late to this party. But if you really wanted to optimize this you need to ditch reflection entirely. I would implement this using dynamic methods.

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.

• Roger: Great! I did the changes, and it really helped my understanding of things I didn't know such as generic type constraints. thanks!
• David: Why NOT static? I was under the impression that if something could be static without changing the meaning or requirements, that it was a good idea to make it so because that will allow you to use it in both static and non-static contexts. Is there a performance hit by doing that or something?
• Mathieu Guindon: @PhatWrat because it's public. What you're saying definitely applies to private members, but public static affects the API, and changing it to an instance method later, is a breaking change. We can only assume that the class this method belongs to is never instantiated and/or is itself static - in any case non-static is always a more object-oriented approach, which is a good idea in an object-oriented paradigm such as C#.
• Kai: "I prefer the former, as explicit scopes are always less error-prone." Did you mean the latter? That's the one with explicit scope ;)
• Zer0: Yes, I agree the constraint is good. But I want to point out to OP that new T() has the exact same performance as Activator.CreateInstace<T>(). They are both very slow.

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

• Roger: Great, could you please show me an example, to clarify my mind a bit? Thanks @hocho
• Roger: Amazing, your code increased my performance test from: 115.000 ms to 85.000 ms thanks!! Another question... Using list.ForEach(item => {}) ... is faster than a simple Foreach(...) ? or using For(int i...)