Reputation: 24063
I have an class Item that represents an item in a list. I have in it function that calls stored procedure that returns datatable and I need to convert the datatable to Array of items. Here is what I do:
public class Item
{
private string _ItemIdDataName = "item_id";
private string _ItemNameDataName = "item_name";
private string _PriceDataName = "price";
public long ItemId { get; set; }
public string ItemName { get; set; }
public float Price { get; set; }
private Item(DataRow row)
{
if (row != null)
{
ItemId = long.Parse(row[_ItemIdDataName].ToString());
ItemName = row[_ItemNameDataName].ToString();
Price = float.Parse(row[_PriceDataName].ToString());
}
}
public Item[] load()
{
DataTable dt=DBHandler.GetItems();//Stored procedure that returns DataTable
Item[] items = new Item[dt.Rows.Count];
for (int i = 0; i < dt.Rows.Count; i++)
{
items[i] = new Item(dt.Rows[i]);
}
return items;
}
}
Am I doing it right? How can I improve this?
Upvotes: 7
Views: 20533
Reputation: 23462
If you're only gonna use it once it probably fine, but if you'll do it a lot you should try to do some more generic stuff. I wrote a blog post about how to write an extension method for DataTable
that creates a list of objects. It works by the convention that the properties in the object should have the same name as the columns in the stored procedure (I would change the name in the stored procedure if I could):
public static class DataTableExtensions
{
public static IList<T> ToList<T>(this DataTable table) where T : new()
{
IList<PropertyInfo> properties = typeof(T).GetProperties().ToList();
IList<T> result = new List<T>();
foreach (var row in table.Rows)
{
var item = CreateItemFromRow<T>((DataRow)row, properties);
result.Add(item);
}
return result;
}
public static IList<T> ToList<T>(this DataTable table, Dictionary<string, string> mappings) where T : new()
{
IList<PropertyInfo> properties = typeof(T).GetProperties().ToList();
IList<T> result = new List<T>();
foreach (var row in table.Rows)
{
var item = CreateItemFromRow<T>((DataRow)row, properties, mappings);
result.Add(item);
}
return result;
}
private static T CreateItemFromRow<T>(DataRow row, IList<PropertyInfo> properties) where T : new()
{
T item = new T();
foreach (var property in properties)
{
property.SetValue(item, row[property.Name], null);
}
return item;
}
private static T CreateItemFromRow<T>(DataRow row, IList<PropertyInfo> properties, Dictionary<string, string> mappings) where T : new()
{
T item = new T();
foreach (var property in properties)
{
if(mappings.ContainsKey(property.Name))
property.SetValue(item, row[mappings[property.Name]], null);
}
return item;
}
}
Now you can just call
var items = dt.ToList<Item>();
or
var mappings = new Dictionary<string,string>();
mappings.Add("ItemId", "item_id");
mappings.Add("ItemName ", "item_name");
mappings.Add("Price ", "price);
var items = dt.ToList<Item>(mappings);
The blog post is here: http://blog.tomasjansson.com/2010/11/convert-datatable-to-generic-list-extension
There are many ways in which you can extend this, you could include some kind of mapping dictionary telling the extension how to map the columns, in that way the names doesn't need to match. Or you can add a list of property names that you would like to exclude in the mapping.
Update: Your object (Item
) you are creating must have a default constructor otherwise the private method won't be able to create it. Since the way the solution works is first to create the object than use the properties that you get from the reflection to set the values of the object.
Update 2: I added the part with mappings dictionary but haven't tried it myself so it might not compile. However, the concept is there and I think it works.
Upvotes: 26
Reputation: 37957
Perfect job for AutoMapper.
Two examples:
http://house9.blogspot.com/2010/11/automapper-datatable-to-list.html
http://www.geekytidbits.com/automapper-with-datatables/
Upvotes: 3
Reputation: 36905
Your private Item constructor and the load()
functions do not seem to belong in your Item
class.
You know, a class should do one thing and one thing well.
So try to
1. refactor out the private c'tor to a say a helper class that simply parses the DataRow and returns an instance of Item
2. and refactor the load()
into a different class that simply uses the above helper method and returns an array of Item object instances
Upvotes: 0
Reputation:
Pretty good, but I have a few suggestions:
Don't cast things ToString to be parsed back to another type. This can cause corruption of your data type, and is slow/inefficient.
Expect and check for null's coming from SQL Server.
So, instead of:
ItemId = long.Parse(row[_ItemIdDataName].ToString());
Try:
ItemId = row.Field<long?>(_ItemIdDataName) ?? value_if_null;
(Add a reference to System.Data.DatasetExtensions to get the Field extension)
Upvotes: 5