9
\$\begingroup\$

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;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to CR! I hope you get great reviews! \$\endgroup\$ – Mathieu Guindon Apr 24 '15 at 0:00
  • \$\begingroup\$ Thanks @Mat'sMug! I've been using stackoverflow.com now I found this community, which looks also great! \$\endgroup\$ – Roger Oliveira Apr 24 '15 at 0:30
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – Dan Lyons Apr 24 '15 at 17:31
  • \$\begingroup\$ but is it faster than the adapter.fill()? \$\endgroup\$ – Roger Oliveira Apr 25 '15 at 23:17
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – Zer0 May 14 '15 at 21:01
10
\$\begingroup\$

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.
\$\endgroup\$
  • 1
    \$\begingroup\$ Great! I did the changes, and it really helped my understanding of things I didn't know such as generic type constraints. thanks! \$\endgroup\$ – Roger Oliveira Apr 24 '15 at 1:30
  • 1
    \$\begingroup\$ 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? \$\endgroup\$ – PhatWrat Apr 24 '15 at 2:08
  • 1
    \$\begingroup\$ @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#. \$\endgroup\$ – Mathieu Guindon Apr 24 '15 at 2:12
  • 1
    \$\begingroup\$ "I prefer the former, as explicit scopes are always less error-prone." Did you mean the latter? That's the one with explicit scope ;) \$\endgroup\$ – Kai Apr 24 '15 at 11:32
  • 2
    \$\begingroup\$ 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. \$\endgroup\$ – Zer0 May 14 '15 at 20:51
5
\$\begingroup\$

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;
    }
\$\endgroup\$
  • \$\begingroup\$ Great, could you please show me an example, to clarify my mind a bit? Thanks @hocho \$\endgroup\$ – Roger Oliveira Apr 24 '15 at 0:34
  • \$\begingroup\$ 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...) \$\endgroup\$ – Roger Oliveira Apr 24 '15 at 1:44

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.