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\$
2
  • \$\begingroup\$ Doesn't the using statement in the ToDataSet function call the Dispose method? \$\endgroup\$
    – Protiguous
    Commented Nov 11, 2014 at 22:45
  • \$\begingroup\$ Yes, however, since we are returning it, it would be pointless to include it. \$\endgroup\$
    – Kevin
    Commented Nov 12, 2014 at 14:27

2 Answers 2

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\$
3
  • \$\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
    Commented Feb 4, 2014 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\$ Commented Feb 4, 2014 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
    Commented Feb 19, 2014 at 0:43
1
\$\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\$
3
  • 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
    Commented Jun 19, 2019 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\$ Commented Jun 20, 2019 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
    Commented Jul 2, 2019 at 12:54

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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