Problem
The goal this method was to emulate the way EF or Dapper return to you concrete POCO classes instead of recordsets when querying your db. Our current code is riddled with iterations over recordsets and I want to consolidate to an extension method that can convert the recordset into an Enumerable<T>
.
Review
I’m concerned about how efficient this will be. When querying the db for a single or handful of records I’m not concerned but we sometimes query for an entire table (i.e. ~100,000 records) and in that case will the iterations in this start to cause performance issues? Is there a better way to resolve the recordsets to classes without iterating? Any other constructive remarks appreciated.
public static IEnumerable<T> AsList<T>(this Recordset rs) where T : new()
{
IEnumerable<PropertyInfo> props = new List<PropertyInfo>(typeof(T).GetProperties()).Where(p => p.CanWrite);
List<T> results = new List<T>();
while (!rs.Eof)
{
T obj = new T();
foreach (PropertyInfo property in props)
{
var val = rs[property.Name].Value;
if (val != null) property.SetValue(obj, Convert.ChangeType(val, property.PropertyType));
}
rs.MoveNext();
results.Add(obj);
}
return results.AsEnumerable();
}
used as…
Recordset rs = new Recordset();
rs.Open("select * from Users", connection)
IEnumerable<User> results = rs.AsList<User>();
Post Review Update
After great feedback from Heslacher and t3chb0t the following code has been finalized. Note there was a change in the requirements and sister method was created As<T>()
to create a single class instance from a recordset. The feedback from the review has been applied to it as well.
public static IList<T> AsList<T>(this Recordset rs) where T : class, new()
{
var results = new List<T>();
if (rs == null) throw new ArgumentNullException("rs");
if (rs.Eof) return results;
IList<PropertyInfo> properties = GetProperties<T>();
while (!rs.Eof)
{
results.Add(CreateInstance<T>(properties, rs));
rs.MoveNext();
}
return results;
}
public static T As<T>(this Recordset rs) where T : class, new()
{
if (rs.Eof) return default(T);
IList<PropertyInfo> properties = GetProperties<T>();
return CreateInstance<T>(properties, rs);
}
private static T CreateInstance<T>(IEnumerable<PropertyInfo> properties, Recordset rs) where T : new()
{
T obj = new T();
foreach (PropertyInfo property in properties)
{
var val = rs[property.Name].Value;
if (val != null && !Convert.IsDBNull(val)) property.SetValue(obj, Convert.ChangeType(val, property.PropertyType));
}
return obj;
}
private static IList<PropertyInfo> GetProperties<T>()
{
return new List<PropertyInfo>(typeof(T).GetProperties().Where(p => p.CanWrite));
}
Solution
public static IEnumerable<T> AsList<T>(this Recordset rs) where T : new()
{
..
List<T> results = new List<T>();
..
return results.AsEnumerable();
}
This might be very inefficient because when I see that a method returns an IEnumerable<T>
I assume its execution is deferred so I automatically do AsList(..).ToList()
to execute it but what it actually does, is to iterate the list twice. First time to create the first list and then to create the second list. You should either create a real deferred method or return the List<T>
so there is no doubt about the result.
IEnumerable<PropertyInfo> props = new List<PropertyInfo>(typeof(T).GetProperties()).Where(p => p.CanWrite);
You are executing this Where
query for each record because the list wrapps only the GetProperties
part (which is an array anyway so I don’t understand why you convert it into a list in the first place – linq can work with arrays). You need to call the ToList
extension after the Where
var properties = typeof(T).GetProperties().Where(p => p.CanWrite).ToList();
I also suggest moving the object creation into a separate method so that you don’t have any nested loops like this:
public static IEnumerable<T> AsList<T>(this Recordset rs) where T : new()
{
var properties = typeof(T).GetProperties().Where(p => p.CanWrite).ToList();
while (!rs.Eof)
{
yield return CreateInstance<T>(properties, rs);
rs.MoveNext();
}
}
private static T CreateInstance<T>(IEnumerable<PropertyInfo> properties, Row row) where T : new()
{
var obj = new T();
foreach (var property in properties)
{
var value = row[property.Name].Value;
if (value != null) property.SetValue(obj, Convert.ChangeType(value, property.PropertyType));
}
return obj;
}
public static IEnumerable<T> AsList<T>(this Recordset rs) where T : new()
You are returning an IEnumerable<T>
not a List
so you should change the name of the method.
- Because the method is
public
you should do proper parameter validation. - The constaraint
new()
should be expanded withclass
so a user of this method would see at first glance thatIEnumerable<int> results = rs.AsList<int>();
won’t work. Without the constraint to use this method only with reference types likeAsList<int>()
you will get for each record in the recordset a value of0
because theprops
variable won’t have any items in it. - You should use the
var
type if its type is obvious from the right hand side of the assignment. - If your class doesn’t have any writeable properties you should return early.
- Because you are returning an
IEnumerable<T>
you could justyield
each createdobj
. - You named a single
PropertyInfo
property
but theList<PropertyInfo>
you namedprops
. You shouldn’t abbreviate variable names because it makes it harder to grasp at first glance what the variable is about. - The mentioned
props
would be better initialized by using theWhere()
on the returned values ofGetProperties()
.
Putting most of the mentioned points together (without method name) would lead to
public static IEnumerable<T> AsList<T>(this Recordset rs) where T : class, new()
{
if (rs == null) { throw new ArgumentNullException("rs"); }
if (rs.EOF) { return Enumerable.Empty<T>(); }
var properties = new List<PropertyInfo>(typeof(T).GetProperties().Where(p => p.CanWrite));
if (properties.Count == 0) { return Enumerable.Empty<T>(); }
return AsListInternal<T>(rs, properties);
}
private static IEnumerable<T> AsListInternal<T>(Recordset rs, List<PropertyInfo> properties) where T : class, new()
{
while (!rs.EOF)
{
var obj = new T();
foreach (var property in properties)
{
var val = rs.Fields[property.Name].Value;
if (val != null)
{
property.SetValue(obj, Convert.ChangeType(val, property.PropertyType));
}
}
yield return obj;
rs.MoveNext();
}
}