8
\$\begingroup\$

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;
        }
    }

}
}
\$\endgroup\$
  • \$\begingroup\$ Doesn't the using statement in the ToDataSet function call the Dispose method? \$\endgroup\$ – Protiguous Nov 11 '14 at 22:45
  • \$\begingroup\$ Yes, however, since we are returning it, it would be pointless to include it. \$\endgroup\$ – Kevin Nov 12 '14 at 14:27
8
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ 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) \$\endgroup\$ – Kevin Feb 4 '14 at 22:04
  • 2
    \$\begingroup\$ 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. \$\endgroup\$ – Jeff Vanzella Feb 4 '14 at 22:27
  • \$\begingroup\$ I never see you in the chat room, even though you are an active user. You should join us sometime! :) \$\endgroup\$ – syb0rg Feb 19 '14 at 0:43
0
\$\begingroup\$

Using this below code, you can convert List<YourClassname> to DataTable:-

List<YourClass> objlist = alldata;
string json = Newtonsoft.Json.JsonConvert.SerializeObject(objlist);
DataTable dt = Newtonsoft.Json.JsonConvert.DeserializeObject<DataTable>(json);

Here, I'm assuming that alldata contains list<YourClass> object and you can also do - objlist.Add(objYourClass). Hope, these codes help you!!!

\$\endgroup\$
  • 1
    \$\begingroup\$ Right, but that also assumes I will include Newtonsoft JSON object as a dependancy to all project using this. No, this is definately not a viable solution to this 5yr old question \$\endgroup\$ – Kevin Jun 19 at 13:46
  • \$\begingroup\$ It is working fine for me. I thing you are having trouble to understand my code. Or your working criteria could be differ from me. \$\endgroup\$ – Love Pandey Jun 20 at 6:07
  • \$\begingroup\$ I'm not having trouble understanding your code mate. You are utilizing a 3rd party reference to do what I have done without. I won't use Newtonsoft.Json's object to do this \$\endgroup\$ – Kevin Jul 2 at 12:54

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.