How to write this in better way?

Question

Hi all. Let's look at this code:

IList<IHouseAnnouncement> list = new List<IHouseAnnouncement>();
var table = adapter.GetData(); //get data from repository object -> DataTable

if (table.Rows.Count >= 1)
{
    for (int i = 0; i < table.Rows.Count; i++)
    {
        var anno = new HouseAnnouncement();
        anno.Area = float.Parse(table.Rows[i][table.areaColumn].ToString());
        anno.City = table.Rows[i][table.cityColumn].ToString();
        list.Add(anno);
    }
  }
  return list;

Is it better way to write this in less code and better fashion (must be :-) )? Maybe using lambda (but let me know how)?

Thanks in advance!

Answer 1

Readability is, to me, preferable to being succinct with your code--as long as performance is not a victim. Also, I am sure that anyone who later has to maintain the code will appreciate it as well.
Even when I am maintaining my own code, I don't want to look at it, say a couple of months later, and think "what the hell was I trying to accomplish"

Answer 2

Just FYI, you're never adding the new HouseAnnouncement to your list, and your loop will never execute for the last row, but I'm assuming those are errors in the example rather than in your actual code.

You could do something like this:

return adapter.GetData().Rows.Cast<DataRow>().Select(row =>
    new HouseAnnouncement()
    {
        Area = float.Parse(row["powierzchnia"].ToString()),
        City = row["miasto"].ToString()
    }).ToList();

I usually go for readability over brevity, but I feel like this is pretty readable.

Note that while you could still cache the DataTable and use table.powierzchniaColumn in the lambda, I eliminated that so that you didn't use a closure that wasn't really necessary (closures introduce substantial complexity to the internal implementation of the lambda, so I avoid them if possible).

If it's important to you to keep the column references as they are, then you can do it like this:

var table = adapter.GetData();

return table.Rows.Cast<DataRow>().Select(row =>
    new HouseAnnouncement()
    {
        Area = float.Parse(row[table.powierzchniaColumn].ToString()),
        City = row[table.miastoColumn].ToString()
    }).ToList();

This will add complexity to the actual IL that the compiler generates, but should do the trick for you.

Answer 3

Your "if statement" is not necessary. Your "for loop" already takes care of that case.

Also, your "for loop" will not execute when the number of your Table Rows is 1. This seems like a mistake, and not by design, but I could be wrong. If you want to fix this, just take out the "-1":

for (int i = 0; i < table.Rows.Count; i++)

Answer 4

Well, for one thing, you appear to have an off-by-one error:

for (int i = 0; i < table.Rows.Count - 1; i++)
{
}

If your table has three rows, this will run while i is less than 3 - 1, or 2, which means it'll run for rows 0 and 1 but not for row 2. This may not be what you intend.

Answer 5

I might do something like this:

var table = adapter.GetData(); //get data from repository object -> DataTable

return table.Rows.Take(table.Rows.Count-1).Select(row => new HouseAnnouncement() {
    Area = float.Parse(row[table.powierzchniaColumn].ToString()),
    City = row[table.miastoColumn].ToString()
}).ToList();

Answer 6

You could do something like this in Linq:

var table = adapter.GetData();
var q = from row in table.Rows.Cast<DataRow>()
        select new HouseAnnouncement() 
           { Area = float.Parse(row[table.areaColumn].ToString()),
             City = row[table.cityColumn].ToString()
           };
return q.ToList();

Answer 7

Can't go much simpler that one for-loop and no if-statements:

var table = adapter.GetData(); //get data from repository object -> DataTable
IList<IHouseAnnouncement> list = new List<IHouseAnnouncement>(table.Rows.Count);

for (int i = 0; i < list.Length; i++)
{
   list[i] = new HouseAnnouncement();
   list[i].Area = float.Parse(table.Rows[i][table.areaColumn].ToString());
   list[i].City = table.Rows[i][table.cityColumn].ToString();
}

return list;

It takes more characters than linq-version, but is parsed faster by programmer's brain. :)