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

2 Answers 2

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\$
7
  • 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
    Commented Apr 24, 2015 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\$
    – David
    Commented Apr 24, 2015 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\$ Commented Apr 24, 2015 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
    Commented Apr 24, 2015 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
    Commented May 14, 2015 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\$
2
  • \$\begingroup\$ Great, could you please show me an example, to clarify my mind a bit? Thanks @hocho \$\endgroup\$
    – Roger
    Commented Apr 24, 2015 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
    Commented Apr 24, 2015 at 1:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.