5
\$\begingroup\$

What would be the most efficent way to calculate this?

var reportLimit = 96*1024;

IEnumerable<int> memoryInUse = 
  things
    .Where(sample => sample.IsOn)
    .Select(sample => sample.MemoryInMb)
    .ToArray();

int totalUnderReportLimit = memoryInUse.Where(ram => ram <= memoryCalcFactor).Sum();
int totalOverEqualReportLimit = memoryInUse.Count(ram => ram > memoryCalcFactor) * memoryCalcFactor;

return totalUnderReportLimit  + totalOverEqualReportLimit;
\$\endgroup\$
2
  • 1
    \$\begingroup\$ If you want to find out what is faster, running benchmarks is exactly what you should do. I'm not sure why are you avoiding that. \$\endgroup\$
    – svick
    Commented Jun 21, 2012 at 23:10
  • \$\begingroup\$ I agree, the ToArray() seems unnecessary here. \$\endgroup\$
    – dreza
    Commented Jun 21, 2012 at 23:16

3 Answers 3

10
\$\begingroup\$

Well here's my take on it. If you want efficient in terms of speed, write a loop. LINQ is not for writing fast code, it's for writing concise code.

memoryInUse is just a collection of the memory counts of all the machines that are on. You then essentially partition those machines by some memoryCalcFactor to do some calculation.

The call to ToArray(), while somewhat helpful (you do the filter/projection once) isn't really needed. You can perform your calculation in one pass and therefore don't need it at all. Look at the following lines in your calculation:

var x = memoryInUse.Where(ram => ram <= memoryCalcFactor).Sum();
var y = memoryInUse.Count(ram => ram > memoryCalcFactor) * memoryCalcFactor;
return x + y;

What are we doing here?

  1. Adding up all the sizes that are less than some factor
  2. Counting all those that are greater than that factor multiplying by that factor.
  3. Adding the previous results

Looking at this at a much higher level, what are we doing here? We're adding up the sizes limiting each size by some factor (a maximum). Write your code to do that.

This is how I'd write it:

var memorySizes = machines
    .Where(machine => machine.IsOn)
    .Select(machine => machine.MemoryInMB);
var result = memorySizes.Sum(size => Math.Max(size, memoryCalcFactor));

Otherwise if you don't want to take the performance hit LINQ will give you and use a loop, here's the equivalent:

var sum = 0;
foreach (var machine in machines)
{
    if (machine.IsOn)
    {
        sum += Math.Max(machine.MemoryInMB, memoryCalcFactor);
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ A .where(...).aggregate(...) should I would think perform an identical single loop... though the aggregate function can feel a little strange to folks not familiar with it. \$\endgroup\$ Commented Jun 25, 2012 at 21:29
  • \$\begingroup\$ @Jimmy: Yeah, I initially started off using aggregate when thinking about this until I realized we were just adding every number in some form. \$\endgroup\$ Commented Jun 25, 2012 at 23:57
3
\$\begingroup\$

I guess one way to potentially speed it up (if the issue was with running over the enumerable twice) would be to use a foreach. However, I don't think it's as readable as your solution;

int countOverLimit = 0;

foreach(var ram in memoryUse)
{
    if(ram <= memoryCalcFactor)
    {
        totalUnderReportLimit += ram;
    }   
    else if(ram > memoryCalcFactor)
    {
         countOverLimit++;
    }
}

totalOverEqualReportLimit = countOverLimit * memoryCalcFactor;

return totalUnderReportLimit + totalOverEqualReportLimit;
\$\endgroup\$
1
\$\begingroup\$

+1 to @Jeff. Staying at low level (if there is not a more effective algorithm) and in languages which does not support LINQ I'd create two methods with single responsibilities (calculateTotalUnderReportLimit and calculateTotalOverEqualReportLimit, for example). If it was a performance bottleneck it still could be optimized but until that it would be easier to read and maintain.

\$\endgroup\$

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.