3
\$\begingroup\$

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));
  }
}
\$\endgroup\$
1
\$\begingroup\$

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;
    }
}
\$\endgroup\$
  • \$\begingroup\$ 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! \$\endgroup\$ – Archangel33 Aug 8 '14 at 14:12
  • \$\begingroup\$ @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 \$\endgroup\$ – Heslacher Aug 8 '14 at 14:15
  • \$\begingroup\$ Oops sorry missed the link. \$\endgroup\$ – Archangel33 Aug 8 '14 at 14:16
  • \$\begingroup\$ 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 \$\endgroup\$ – Archangel33 Aug 8 '14 at 17:19
  • \$\begingroup\$ @Archangel33 You are right. I have edited my answer. \$\endgroup\$ – Heslacher Aug 8 '14 at 17:22
2
\$\begingroup\$
  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();
}
\$\endgroup\$
  • \$\begingroup\$ 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 :) \$\endgroup\$ – paritosh Aug 8 '14 at 9:08
  • \$\begingroup\$ I don't want to steal the name, but the idea behind. \$\endgroup\$ – Heslacher Aug 8 '14 at 9:09
  • \$\begingroup\$ oh okay gotcha lolz, \$\endgroup\$ – paritosh Aug 8 '14 at 9:12
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – Archangel33 Aug 8 '14 at 14:01

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.