tags:

views:

95

answers:

3

The following method is pretty simple, I'm trying to determine a line-item rate by matching up another property of the line-item with a lookup from a parent object. There's a few things I don't like about it and am looking for elegant solutions to either make the method smaller, more efficient, or both. It works in it's current state and it's not like it's noticeably inefficient or anything. This isn't mission critical or anything, more of a curiosity.

    private decimal CalculateLaborTotal()
    {
        decimal result = 0;

        foreach (ExtraWorkOrderLaborItem laborItem in Labor)
        {
            var rates = (from x in Project.ContractRates where x.ProjectRole.Name == laborItem.ProjectRole.Name select x).ToList();
            if (rates != null && rates.Count() > 0)
            {
                result += laborItem.Hours * rates[0].Rate;
            }
        }
        return result;
    }

I like the idea of using List<T>.ForEach(), but I was having some trouble keeping it succinct enough to still be easy to read/maintain. Any thoughts?

+4  A: 

Something like this should do it (untested!):

var result = 
    (from laborItem in Labor
     let rate = (from x in Project.ContractRates 
                 where x.ProjectRole.Name == laborItem.ProjectRole.Name 
                 select x).FirstOrDefault()
     where rate != null
     select laborItem.Hours * rate.Rate).Sum();

Or (assuming only one rate can match) a join would be even neater:

var result = 
    (from laborItem in Labor
     join rate in Project.ContractRates
         on laborItem.ProjectRole.Name equals rate.ProjectRole.Name
     select laborItem.Hours * rate.Rate).Sum();
Greg Beech
yup, only one rate should match. I like the join option, I'll play with that a bit.
mannish
I know this question is rather subjective, but I really liked how the join in your answer made my code more succinct while maintaining readability. I'm newish to linq and this was a good example to build on.
mannish
A: 

Omit the rates != null check - A linq query may be empty but not null. If you just need the first element of a list, use List.First or List.FirstOrDefault.

There is no advantage in using List<T>.ForEach.

Dario
+2  A: 

Okay, well how about this:

// Lookup from name to IEnumerable<decimal>, assuming Rate is a decimal
var ratesLookup = Project.ContractRates.ToLookup(x => x.ProjectRole.Name,
                                                 x => x.Rate);

var query = (from laborItem in Labor
             let rate = ratesGroup[laborItem].FirstOrDefault()
             select laborItem.Hours * rate).Sum();

The advantage here is that you don't need to look through a potentially large list of contract rates every time - you build the lookup once. That may not be an issue, of course.

Jon Skeet