6
\$\begingroup\$

I have implemented the code to convert datareader to list of objects. My intention was to not to use column name string literals while reading the datareader.

public static IEnumerable<T> GetListFromDataReader<T>(IDataReader reader) where T : new()
{
    var properties = typeof(T).GetProperties();

    var modelProperties = new List<string>();
    var columnList = (reader.GetSchemaTable().Select()).Select(r => r.ItemArray[0].ToString());
    while (reader.Read())
    {
        var element = Activator.CreateInstance<T>();
        Dictionary<string, string> dbMappings = DBColumn(element);
        string columnName;
        foreach (var f in properties)
        {

            if (!columnList.Contains(f.Name) && !dbMappings.ContainsKey(f.Name))
                continue;
            columnName = dbMappings.ContainsKey(f.Name) ? dbMappings[f.Name] : f.Name;
            var o = (object)reader[columnName];

            if (o.GetType() != typeof(DBNull)) f.SetValue(element, ChangeType(o, f.PropertyType), null);
        }
        yield return element;
    }

}

public static object ChangeType(object value, Type conversion)
{
    var t = conversion;

    if (t.IsGenericType && t.GetGenericTypeDefinition().Equals(typeof(Nullable<>)))
    {
        if (value == null)
        {
            return null;
        }

        t = Nullable.GetUnderlyingType(t); ;
    }

    return Convert.ChangeType(value, t);
}

public static Dictionary<string,string> DBColumn<T>(T item) where T:new()
{

    Dictionary<string, string> dbMappings = new Dictionary<string, string>();
    var type = item.GetType();
    var properties = type.GetProperties();
    foreach (var property in properties)
    {
        var attributes = property.GetCustomAttributes(false);
        var columnMapping = attributes
    .FirstOrDefault(a => a.GetType() == typeof(DbColumnAttribute));
        if (columnMapping != null)
        {
            dbMappings.Add(property.Name, ((DbColumnAttribute)columnMapping).Name);
        }
    }
    return dbMappings;
}

Can somebody help me by providing a review? I am doubtful of too much usage of reflections in loops would downgrade the performance. Or should I compromise and better to go with string literals itself while reading?

\$\endgroup\$
4
\$\begingroup\$

<clippy>It appears you are writing an Object/Relational Mapper! Would you like some help?</clippy>

Yes, a lot of reflection in loops can be bad thing since reflection is known for its slowness. In a situation like this, it's best to cache the results as your types will not be changing during the lifetime of the program. Simple fix:

    private static readonly IDictionary<Type, IDictionary<string, string>> typeMappings =
        new Dictionary<Type, IDictionary<string, string>>();

    public static IDictionary<string, string> DBColumn<T>() where T : new()
    {
        var type = typeof(T);
        IDictionary<string, string> databaseMappings;

        if (typeMappings.TryGetValue(type, out databaseMappings))
        {
            return databaseMappings;
        }

        databaseMappings = new Dictionary<string, string>();
        foreach (var property in type.GetProperties())
        {
            var columnMapping = property
                .GetCustomAttributes(false)
                .FirstOrDefault(attribute => attribute is DbColumnAttribute);

            if (columnMapping != null)
            {
                databaseMappings.Add(property.Name, ((DbColumnAttribute)columnMapping).Name);
            }
        }

        typeMappings.Add(type, databaseMappings);
        return databaseMappings;
    }

You'll notice it uses a static dictionary to hold the mappings between the type and the data column mappings, so it should only be done once per type. That'll be your performance boost right there. You'll only take the hit the first time through and not after that.

Also, I did change some variable names and coded to interfaces (IDictionary instead of Dictionary) and take note that the method doesn't need a parameter any more: the T generic type parameter gives me the type rather than doing .GetType() on the parameter.

Hope this helps you!

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Hi Jesse C. Slicer: Appreciate for you time. Can you suggest me, whether this would be good practice to retrieve data from the datareader through above fashion, where the project is involved lot of CRUD ops. \$\endgroup\$ – gee'K'iran Jul 30 '14 at 6:48
  • 1
    \$\begingroup\$ Personally, I'd forego any usage of DataReader and any other ADO.NET operations and use a time-tested O/RM such as Entity Framework, NHibernate or ORMLite. It does what you're looking to do and lots more. \$\endgroup\$ – Jesse C. Slicer Jul 30 '14 at 13:23
  • \$\begingroup\$ Thanks Jesse. I am also a fan of ORMs and thats the reason I am trying to avoid reading the datareader values manually by passing string keys. However, the application, which I started working recently is an ongoing one since 2 years and we are not at the place where we can revamp the whole Ado.Net stuff. \$\endgroup\$ – gee'K'iran Jul 31 '14 at 7:34
5
\$\begingroup\$

I can suggest a number of improvements you can make to the code:

  1. You're calling GetSchemaTable to get column names. To simply fetch column names you don't need, there is a faster alternative:

    var columns = Enumerable.Range(0, reader.FieldCount).Select(reader.GetName).ToList();
    
  2. You're not computing the result of your select query to get column names. Select extension method is lazily evaluated which means it is executed every time you call it. Instead you should materialize the query by calling ToList() or something like that (as shown above).

  3. You should make use of expression trees to generate compile code at run-time. You can make use of such code in many ways:

    • Db operations are expensive. Add reflection to it, it gets worse. Expression trees help you get the benefit of reflection at native speed once you compile some logic. You should replace all reflection logic with expression trees. One example

    • Activator.CreateInstance<T>() uses reflection behind the scenes which is slower. Expression trees can make that case faster too. See

    • reader["columnName"] will be marginally slower than reader[columnIndex] though it will ultimately depend on the ADO.NET connector implementation. It is safe to assume the latter is faster. You could spit out code at run-time to cache the ordinal values of fields and use it to query the reader like shown here.

  4. Your columnCollection.Contains(string columnName) is case-sensitive. Make it case-insensitive since property naming convention can differ from db column naming conventions.

  5. You don't need the casting here:

    (object)reader[columnName];
    

    What you get returned from reader is already object. Just code readability matters.

  6. You're not assigning the column value to property when reader returns DBNull. This can give rise to oddities at times. Imagine you have an unfortunately designed class like this:

    class User { public User() { Property = "42"; } }
    

    Assume your db value for Property column is DBNull. If you don't assign anything in your mapping code then the returned User object will have a default value of "42" for Property where as in db the corresponding field will be DBNull which indicates null.

  7. As Jesse has pointed in his answer, you're using reflection code to map inside the loop. I would take it outside. But unlike him I wouldn't recommend caching in a static dictionary permanently. I would like to optimize for memory too.

  8. You're doing double dictionary lookup in this line:

    columnName = dbMappings.ContainsKey(f.Name) ? dbMappings[f.Name] : f.Name;
    

    Use TryGetValue instead.

  9. No need to check twice for column name like this:

    if (!columnList.Contains(f.Name) && !dbMappings.ContainsKey(f.Name))
        continue;
    columnName = dbMappings.ContainsKey(f.Name) ? dbMappings[f.Name] : f.Name;
    

    In fact, no need to check for presence columns in column collection at all; just loop column collection straight away instead of property collection. Or even better, take expression route, no need of any looping, you could assign to variables statement by statement.

  10. You're doing GetProperties twice. Instead call it once, and cache it in dictionary. So your dictionary must look like Dictionary<string, PropertInfo> rather than Dictionary<string, string>. Or make use of ordinals, so your dictionary looks like Dictionary<int, PropertyInfo>.

  11. Since you're constraining the T to new() you can just call new T() rather than Activator.CreateInstance<T>(). This is simpler syntax.

  12. The columnName value you use here:

    (object)reader[columnName];
    

    is obtained from property names. If there is a case difference between property name and the actual field name in the reader the lookup will be slower, depending on the implementation of ADO.NET connector. It's better to use the column name you get from the reader itself. Or even better, just use ordinals to query (i.e. indices).

  13. Not all properties are settable. It is better to check it beforehand.

  14. There's a deeper problem here with IDisposables. You have to use the using statement to properly dispose your connection, command and reader objects. When you leave the method which has using keyword used, your objects (especially the reader) will be disposed and hence you wont be able to operate on reader outside your method. Your alternative is to not use using at all and dispose off everything manually after you're done. This wont look good. I would rather iterate the reader in the method which initializes the reader rather than in the mapper class, and return using the yield keyword which blocks disposing off reader object until enumeration.


The gist of it (except expression trees which is too much plumbing, but I have given a reference link so that you can work on) would look like:

public class DataReaderMapper<T> where T : new()
{
    Dictionary<int, PropertyInfo> mappings;

    public DataReaderMapper(IDataReader reader)
    {
        this.mappings = Mappings(reader);
    }

    // int part is column indices (ordinals)
    static Dictionary<int, PropertyInfo> Mappings(IDataReader reader)
    {
        var columns = Enumerable.Range(0, reader.FieldCount);
        var properties = typeof(T)
                        .GetProperties()
                        .Select(prop => new
                        {
                            prop,
                            attr = prop.GetCustomAttributes(typeof(DbColumnAttribute)).FirstOrDefault()
                        })
                        .Select(x => new
                        {
                            name = x.attr == null ? x.prop.Name : ((DbColumnAttribute)x.attr).Name,
                            x.prop
                        });
        return columns
              .Join(properties, reader.GetName, x => x.name, (index, x) => new
              {
                  index,
                  prop = !x.prop.CanWrite ? null : x.prop
              }, StringComparer.InvariantCultureIgnoreCase)
             .Where(x => x.prop != null) // only settable properties accounted for
             .ToDictionary(x => x.index, x => x.prop);
    }

    public T MapFrom(IDataRecord record)
    {
        var element = new T();
        foreach (var map in mappings)
            map.Value.SetValue(element, ChangeType(record[map.Key], map.Value.PropertyType));

        return element;
    }

    static object ChangeType(object value, Type targetType)
    {
        if (value == null || value == DBNull.Value)
            return null;

        return Convert.ChangeType(value, Nullable.GetUnderlyingType(targetType) ?? targetType);
    }
}

You can call it like:

using (var reader = cmd.ExecuteReader())
{
    var mapper = new DataReaderMapper<T>(reader);
    while (reader.Read())
        yield return mapper.MapFrom(reader);
}

I believe this code is overall shorter, but personally I think you should go for expressions instead of reflection, or even better try a tried and tested ORM.

I'm passing reader object to the constructor of mapper class as well as in the MapFrom method for a reason. Designing a clean API is little difficult here considering reader form both IDataReader (the enumerable) as well as IDataRecord (the result of enumeration). The reason I pass it separately is performance. I dont cache anything, so memory management is left to client. If you don't want the mapping to happen every time you can just cache the mapper instance and dispose it off when you don't want anymore. For e.g. you could do this:

 DataReaderMapper<T> mapper;
 using (var reader = cmd.ExecuteReader())
 {
     mapper = new DataReaderMapper<T>(reader); // initialized once
     while (reader.Read())
         yield return mapper.MapFrom(reader);
 }

 using (var reader = cmd.ExecuteReader())
 {
     while (reader.Read())
         yield return mapper.MapFrom(reader); // no need of further initialization
 }

To use it at a later stage you can cache mapper in some dictionary detailing which out will be too much for now.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ Wonderful... Thanks so much, for your efforts.. \$\endgroup\$ – gee'K'iran Dec 3 '15 at 9:24

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.