Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Can someone help me improve this code? I and trying to have a couple extension methods to convert strongly-typed lists to a DataSet and DataTable respectively.

But... being so green to C#, I am not sure this is the most efficient way to do this.

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Reflection;

namespace o7th.Class.Library.Data
{
/// <summary>
/// List to DataTable Converter
/// </summary>
public static class ListConverter
{

    /// <summary>
    /// Convert our List to a DataTable
    /// </summary>
    /// <typeparam name="T"></typeparam>
    /// <param name="data"></param>
    /// <returns>DataTable</returns>
    public static DataTable ToDataTable<T>(this IList<T> data)
    {
        PropertyDescriptorCollection props = TypeDescriptor.GetProperties(typeof(T));
        object[] values = new object[props.Count];
        using (DataTable table = new DataTable())
        {
            long _pCt = props.Count;
            for (int i = 0; i < _pCt; ++i)
            {
                PropertyDescriptor prop = props[i];
                table.Columns.Add(prop.Name, prop.PropertyType);
            }
            foreach (T item in data)
            {
                long _vCt = values.Length;
                for (int i = 0; i < _vCt; ++i)
                {
                    values[i] = props[i].GetValue(item);
                }
                table.Rows.Add(values);
            }
            return table;
        }
    }

    /// <summary>
    /// Convert our List to a DataSet
    /// </summary>
    /// <typeparam name="T"></typeparam>
    /// <param name="list"></param>
    /// <returns>DataSet</returns>
    public static DataSet ToDataSet<T>(this IList<T> list)
    {
        Type elementType = typeof(T);
        using (DataSet ds = new DataSet())
        {
            using (DataTable t = new DataTable())
            {
                ds.Tables.Add(t);
                //add a column to table for each public property on T
                PropertyInfo[] _props = elementType.GetProperties();
                foreach (PropertyInfo propInfo in _props)
                {
                    Type _pi = propInfo.PropertyType;
                    Type ColType = Nullable.GetUnderlyingType(_pi) ?? _pi;
                    t.Columns.Add(propInfo.Name, ColType);
                }
                //go through each property on T and add each value to the table
                foreach (T item in list)
                {
                    DataRow row = t.NewRow();
                    foreach (PropertyInfo propInfo in _props)
                    {
                        row[propInfo.Name] = propInfo.GetValue(item, null) ?? DBNull.Value;
                    }
                    t.Rows.Add(row);
                }
            }
            return ds;
        }
    }

}
}

Code Update

using System;
using System.Collections.Generic;
using System.Data;
using System.Reflection;

namespace o7th.Class.Library.Data
{
/// <summary>
/// List to DataTable Converter
/// </summary>
public static class ListConverter
{

    /// <summary>
    /// Convert our IList to a DataTable
    /// </summary>
    /// <typeparam name="T"></typeparam>
    /// <param name="list"></param>
    /// <returns>DataTable</returns>
    public static DataTable ToDataTable<T>(this IEnumerable<T> list)
    {
        Type elementType = typeof(T);
        using (DataTable t = new DataTable())
        {
            PropertyInfo[] _props = elementType.GetProperties();
            foreach (PropertyInfo propInfo in _props)
            {
                Type _pi = propInfo.PropertyType;
                Type ColType = Nullable.GetUnderlyingType(_pi) ?? _pi;
                t.Columns.Add(propInfo.Name, ColType);
            }
            foreach (T item in list)
            {
                DataRow row = t.NewRow();
                foreach (PropertyInfo propInfo in _props)
                {
                    row[propInfo.Name] = propInfo.GetValue(item, null) ?? DBNull.Value;
                }
                t.Rows.Add(row);
            }
            return t;
        }
    }

    /// <summary>
    /// Convert our IList to a DataSet
    /// </summary>
    /// <typeparam name="T"></typeparam>
    /// <param name="list"></param>
    /// <returns>DataSet</returns>
    public static DataSet ToDataSet<T>(this IEnumerable<T> list)
    {
        using (DataSet ds = new DataSet())
        {
            ds.Tables.Add(list.ToDataTable());
            return ds;
        }
    }

}
}
share|improve this question
    
Doesn't the using statement in the ToDataSet function call the Dispose method? –  R H Nov 11 at 22:45
    
Yes, however, since we are returning it, it would be pointless to include it. –  Kevin Nov 12 at 14:27

1 Answer 1

up vote 5 down vote accepted

There are a few things I notice right off the bat.

  1. Your variable names while not bad, could be better. For instance props -> properties. Stuff like this makes the code easier to read.

  2. You have the properties, why not use a foreach loop to fill the datatable (you did it in ToDataSet)

  3. the _ prefex should be used for class variables, not local variables.

  4. try using var when declaring obvious variable types var row = t.NewRow()

  5. There is no error checking when you are filling the values in the data table. What happens if it is not a class (int, double, long)? You could force the generic to be a class by adding where T : class.

  6. Why don't you use the ToDataTable method to create the table in the ToDataSet method? This will eliminate duplicate code, and have 1 point of failure/modification as required. As an aside, I would use the code from ToDataSet to create your DataTable, as it is written better.

  7. While I applaud your use of it, I'm not sure the using syntax is appropriate here. I would move that to where these methods are being called using (var dt = list.ToDataTable()) Having it here will more than likely cause unexpected things to happen in your code.

  8. I would make these extend IEnumerable<T> as that will make them way more useful by not limiting them to IList<T>.

I do like your use of white space and indentation, so good job on that. The extra indentation will be removed when the using statements are removed. I also like the name of your methods, very clear and concise to their intent.

share|improve this answer
    
Thank you for the comments and suggestions. :) Couple questions/comments. #5 not worried about that just yet, but it will go in. #4. I've always been under the impression that it is more efficient to properly declare my variables, what would the benefit be of making it a variable type rather than a DataRow? (updated the q, with new code) –  Kevin Feb 4 at 22:04
2  
var actually is strongly typed in C#. It picks up the type you are assigning it. I like it because it cleans the code up a little more. –  Jeff Vanzella Feb 4 at 22:27
    
I never see you in the chat room, even though you are an active user. You should join us sometime! :) –  syb0rg Feb 19 at 0:43

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.