I can suggest a number of improvements you can make to the code:
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();
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).
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.
Your columnCollection.Contains(string columnName) is case-sensitive. Make it case-insensitive since property naming convention can differ from db column naming conventions.
You don't need the casting here:
(object)reader[columnName];
What you get returned from reader is already object. Just code readability matters.
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.
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.
You're doing double dictionary lookup in this line:
columnName = dbMappings.ContainsKey(f.Name) ? dbMappings[f.Name] : f.Name;
Use TryGetValue instead.
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.
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>.
Since you're constraining the T to new() you can just call new T() rather than Activator.CreateInstance<T>(). This is simpler syntax.
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).
Not all properties are settable. It is better to check it beforehand.
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.
