DataTable to given type T simple mapper

Question

I threw this together and would love any suggestions on how to improve on this simple DataTable to given Type T mapper. Anything from coding conventions to speed optimizations or "you're doing something stupid".

public class test {
 static ISXLog log = new XLog();
 public void runTest() {
 var data = RetrieveDataSet(/*parameters not important*/);
 AddressType[] addr = mapObjects<AddressType>(data.Tables[0]); 
 }
 // overload for optional function parameter
 public static T[] mapObjects<T>(DataTable dt ) where T : new(){
 return mapObjects<T>(dt, (p => p.IsCollection()));
 }
 // mapObjects from DataTable to Type T
 public static T[] mapObjects<T>(DataTable dt, Func<PropertyInfo,bool>propRestriction) where T : new() {
 T[] newobjs;
 var Rows = dt.Rows;
 newobjs = new T[Rows.Count];
 var MField = typeof(DataRowExtensions)
 .GetMethod(@"Field", new[] { typeof(DataRow), typeof(string) });
 for (int i = 0; i < Rows.Count; i++) {
 DataRow dbobj = Rows[i];
 var obj = newobjs[i] = new T();
 var objProps = obj.GetType().GetProperties();
 foreach (var prop in objProps) {
 try {
 if (!dbobj.Table.Columns.Contains(prop.Name) || propRestriction(prop)) {
 log.Debug("Nothing to set for property: {0}", prop.Name);
 }else{
 MField = MField.GetGenericMethodDefinition()
 .MakeGenericMethod(prop.PropertyType);
 var objval = MField.Invoke(null, new object[] { dbobj, prop.Name });
 prop.SetValue(obj, objval, null);
 log.Debug("Set property '{0}' to value '{1}'", prop.Name, objval);
 }
 }
 catch (Exception e) {
 log.DebugException("Error occured while trying to set prop: " + prop.Name, e);
 }
 }
 }
 return newobjs;
}
 public static class PropertyInfoExtensions
 {
 public static bool IsCollection(this PropertyInfo property) {
 return (!typeof(String).Equals(property.PropertyType) &&
 typeof(IEnumerable).IsAssignableFrom(property.PropertyType));
 }
}

Answer 1

Naming
Methodnames should be using PascalCase based on the Method Naming Guidlines so

public static T[] mapObjects<T>()

should be

public static T[] MapObjects<T>()

or better, as the method is mapping datarows of a datatable to objects T

public static T[] MapTo<T>()

Variables should be using camelCase and should be as descriptive as possible, so they are readable/understandable for Mr.Maintainer.

T[] newobjs;
var Rows = dt.Rows;
DataRow dbobj = Rows[i];

should be e.g

T[] newObjs;
var rows = dt.Rows;
DataRow row = rows[i]; 

Bug

var Rows = dt.Rows;
newobjs = new T[Rows.Count];

You are creating a new array of T whichs dimension is 1 to big. This should be

var Rows = dt.Rows;
newobjs = new T[Rows.Count -1]; 

but now you also need to check for the Rows.Countproperty if it is 0.

Avoidable problems

• As the method having a Func<PropertyInfo, bool> property is public, you should check if the parameter is null
• A retrieved columnvalue also can be DBNull, insert a DBNull check

Optimization

• As the columns of the datatable won't change you don't need to query them for each
  datarow
• This should be an extension method for DataTable

After taking all this, the refactored methods will result in

public class TestDataTableExtensions
{
 public static void runTest()
 {
 var data = RetrieveDataSet(/*parameters not important*/);
 AddressType[] addr = data.Tables[0].MapTo<AddressType>();
 }
} 

---

public static class DataTableExtensions
{
 private static ILogger log = Logger.GetLogger();
 public static T[] MapTo<T>(this DataTable dt) where T : new()
 {
 return dt.MapTo<T>(p => p.IsCollection());
 }
 public static T[] MapTo<T>(this DataTable dt, Func<PropertyInfo, bool> propertyRestriction) where T : new()
 {
 T[] mappedObjects = null;
 var rows = dt.Rows;
 if (rows.Count == 0)
 {
 return mappedObjects;
 }
 mappedObjects = new T[rows.Count - 1];
 var methodInfo = typeof(DataRowExtensions)
 .GetMethod(@"Field", new[] { typeof(DataRow), typeof(string) });
 DataColumnCollection columns = dt.Columns;
 for (int i = 0; i < rows.Count; i++)
 {
 DataRow row = rows[i];
 var currentObject = mappedObjects[i] = new T();
 var properties = currentObject.GetType().GetProperties();
 foreach (var property in properties)
 {
 String propertyName = property.Name;
 try
 {
 if (!columns.Contains(propertyName) || 
 (propertyRestriction != null && propertyRestriction(property)))
 {
 log.Debug("Nothing to set for property: {0}", propertyName);
 }
 else
 {
 methodInfo = methodInfo.GetGenericMethodDefinition()
 .MakeGenericMethod(property.PropertyType);
 var objval = methodInfo.Invoke(null, new object[] { row, propertyName });
 property.SetValue(currentObject,
 DBNull.Value.Equals(value) ? value : null,
 null);
 log.Debug("Set property '{0}' to value '{1}'", propertyName, objval);
 }
 }
 catch (Exception e)
 {
 log.DebugException("Error occured while trying to set prop: " + property.Name, e);
 }
 }
 }
 return mappedObjects;
 }
} 

---

public static class PropertyInfoExtensions
{
 public static bool IsCollection(this PropertyInfo property)
 {
 return (!typeof(String).Equals(property.PropertyType) &&
 typeof(IEnumerable).IsAssignableFrom(property.PropertyType));
 }
} 

---

public interface ILogger
{
 void Debug(String message, params Object[] objects);
 void DebugException(String message, Exception e);
}
public class Logger:ILogger
{
 private static ISXLog logger = new XLog();
 public static ILogger GetLogger()
 {
 return new Logger();
 }
 void ILogger.Debug(string message, params object[] objects)
 {
 logger.Debug(message, objects);
 }
 void ILogger.DebugException(string message, Exception e)
 {
 logger.DebugException(message, e);
 }
} 

I just couldn't resist to beautyfy this and therefor I lend the following parts from paritosh's answer.
If you, the OP, consider to mark my answer because of the parts lend from paritosh's answer then don't do it.

var mappedObjectProperties = typeof(T).GetProperties()
 .Where(elem => !propRestriction(elem)).ToList();
var mappedObjectCollection = new List<T>();

After renaming mappedObjectProperties to properties (also it is a good name), adjusting propRestriction to fit the parametername of our implementation and renaming mappedObjectCollection to mappedObjects, we need to add a check if propertyRestriction != null.

IEnumerable<PropertyInfo> properties =
 ((propertyRestriction != null) ?
 typeof(T).GetProperties().Where(elem => !propertyRestriction(elem)) :
 typeof(T).GetProperties());

But as always we can do better. Why shouldn't we restrict the properties to only the columnnames of the datatable ?

IEnumerable<PropertyInfo> properties =
 ((propertyRestriction != null) ?
 typeof(T).GetProperties().Where(elem => !propertyRestriction(elem)) :
 typeof(T).GetProperties())
 .Where(prop => dt.Columns.Contains(prop.Name));

Next we add a check if either !properties.Any() or dt.Rows.Count == 0 and if one of these checks will be true, we are finished.

But, why shouldn't we add an extension method for a DataRow ? We just do it. As we need the name of the property to access the value of the datacolumn and also for logging we add a String propertyName

 public static T MapTo<T>(this DataRow dataRow, IEnumerable<PropertyInfo> properties) where T : new()
 {
 T currentObject = new T();
 foreach (PropertyInfo property in properties)
 {
 String propertyName = property.Name;
 try
 {
 var value = dataRow[propertyName];
 property.SetValue(currentObject,
 DBNull.Value.Equals(value) ? value : null,
 null);
 log.Debug("Set property '{0}' to value '{1}'", propertyName, value);
 }
 catch (Exception e)
 {
 log.DebugException("Error occured while trying to set prop: " + property.Name, e);
 }
 }
 return currentObject;
 }
}

Now we can simplify the DataTable's extension method to

 public static T[] MapTo<T>(this DataTable dt, Func<PropertyInfo, bool> propertyRestriction) where T : new()
 {
 IList<T> mappedObjects = new List<T>();
 IEnumerable<PropertyInfo> properties =
 ((propertyRestriction != null) ?
 typeof(T).GetProperties().Where(elem => !propertyRestriction(elem)) :
 typeof(T).GetProperties())
 .Where(prop => dt.Columns.Contains(prop.Name));
 if (!properties.Any() || dt.Rows.Count == 0) { return mappedObjects.ToArray(); }
 foreach (DataRow dataRow in dt.Rows)
 {
 mappedObjects.Add(dataRow.MapTo<T>(properties));
 }
 return mappedObjects.ToArray();
 }

Here we go..

public static class DataMappingExtensions
{
 private static ILogger log = Logger.GetLogger();
 public static T[] MapTo<T>(this DataTable dt) where T : new()
 {
 return dt.MapTo<T>(null);
 }
 public static T[] MapTo<T>(this DataTable dt, Func<PropertyInfo, bool> propertyRestriction) where T : new()
 {
 IList<T> mappedObjects = new List<T>();
 IEnumerable<PropertyInfo> properties =
 ((propertyRestriction != null) ?
 typeof(T).GetProperties().Where(elem => !propertyRestriction(elem)) :
 typeof(T).GetProperties())
 .Where(prop => dt.Columns.Contains(prop.Name));
 if (!properties.Any() || dt.Rows.Count == 0) { return mappedObjects.ToArray(); }
 foreach (DataRow dataRow in dt.Rows)
 {
 mappedObjects.Add(dataRow.MapTo<T>(properties));
 }
 return mappedObjects.ToArray();
 }
 public static T MapTo<T>(this DataRow dataRow, IEnumerable<PropertyInfo> properties) where T : new()
 {
 T currentObject = new T();
 foreach (PropertyInfo property in properties)
 {
 String propertyName = property.Name;
 try
 {
 var value = dataRow[propertyName];
 property.SetValue(currentObject,
 !DBNull.Value.Equals(value) ? value : null,
 null);
 log.Debug("Set property '{0}' to value '{1}'", propertyName, value);
 }
 catch (Exception e)
 {
 log.DebugException("Error occured while trying to set prop: " + property.Name, e);
 }
 }
 return currentObject;
 }
}

• Archangel33: I appreciate the time and effort put into your answer. Why Pascal? I thought only classes should be Pascal and everything else should be cammel. I can't change the logger im using but I think it does have your suggestions I will change my usage. Thank you for your examples I will try to learn from them. It may take a few more poundings but I will learn. :) thank you!
• Heslacher: @Archangel33 only Parameter and Protected instance fields should be camelCased check this link down the page http://msdn.microsoft.com/en-us/library/x2dbyw72%28v=vs.71%29.aspx
• Archangel33: Oops sorry missed the link.
• Archangel33: I believe your code for the DBNull check is missing a !. It should be !DBNull.Value.Equals(value) ? value : null or DBNull.Value.Equals(value) ? null : value instead of DBNull.Value.Equals(value) ? value : null
• Heslacher: @Archangel33 You are right. I have edited my answer.

Answer 2

1. You can minimize you code taking out the filtering out side of loop.
2. Avoid catching exception inside a loop if it needed ,(it seems like it cannot be avoided here)
3. No need to invoke datarow extension method to get value , you can get it directly.
4. Naming of variable is really bad.

This is first round of re factoring.

public static T[] MapObjects<T>(DataTable dataTable, Func<PropertyInfo, bool> 
 propRestriction) where T : new()
{
var mappedObjectProperties = typeof(T).GetProperties()
 .Where(elem => !propRestriction(elem)).ToList();
var mappedObjectCollection = new List<T>();
foreach (var dataRow in dataTable.AsEnumerable())
{
 var mappedObject = new T();
 foreach (var mappedObjectProperty in mappedObjectProperties)
 {
 if (dataTable.Columns.Contains(mappedObjectProperty.Name))
 {
 var value = dataRow[mappedObjectProperty.Name];
 mappedObjectProperty.SetValue(mappedObject, value, null);
 }
 else
 {
 log.Debug("Nothing to set for property: {0}", prop.Name);
 }
 }
 mappedObjectCollection.Add(mappedObject);
 }
 return mappedObjectCollection.ToArray();
}

• Paritosh: ha ha it took me 10 min to finalize that name , still I am not feeling good, you have any suggestion , I would love to steal from you :)
• Heslacher: I don't want to steal the name, but the idea behind.
• Paritosh: oh okay gotcha lolz,
• Archangel33: Thank you for your critique, I don't know where my head was when trying to invoke the DataTable's extension method. O_o Looks like I have to work on my naming conventions and try for better ones more naturally.