Tests using .Count() on Lists

The test

PublicQuestion_ComplexTest

is taking around 1600ms (1.6 seconds) to run, but used to take 92ms. I have a feeling this is because of recent code changes that include .Count() in many places within these 2 functions:

RemoveSmallerLengthNumberListsThatContainSameNumberCombinationAsLargerLengthCombinations

and

CropNumbersToBeWithinAllowances

The goal is for the code to run much more efficiently, hopefully under 100ms. Any suggestions?

```
[Test]
public void PublicQuestion_SimpleTest()
{
// Fixed values
var group1 = new int[] { 1 };
var group2 = new int[] { 4 };
var group3 = new int[] { 6 };

// Selection values
var group4 = new int[] { 1, 2, 3 };
var group5 = new int[] { 3, 4 };
var group6 = new int[] { 3, 4, 5 };
var group7 = new int[] { 1, 5 };

var groups = new List>()
{
group1, //1
group1, //1
group2, //4
group3, //6
group4, //1, 2, 3
group5, //3, 4
group6, //3, 4, 5
group7 //1, 5
};

var allowances = new Dictionary()
{
{1, 999},
{2, 1},
{3, 2},
{4, 2},
{5, 1},
{6, 1}
};

var finder = new PickOneNumberFromEachGroupCombinationFinder(groups, allowances);
var watch = System.Diagnostics.Stopwatch.StartNew();
IList> results = finder.GetAll();
watch.Stop();
Assert.AreEqual(14, results.Count);
Assert.IsTrue(watch.ElapsedMilliseconds combinationStringed = new List();
foreach (var group in results)
{
StringBuilder sb = new StringBuilder();
foreach (var number in group)
{
sb.Append(number + ".");
}
combinationStringed.Add(sb.ToString());
}
StringBuilder sbOrderedResults = new StringBuild

In CropNumbersToBeWithinAllowances() you do

newCombination.Where(x => x == currentValue).Count()

inside the for loop. newCombination is of type List so calling Count() on that would fall back to Count (as Anthony Pegram already mentioned in the comments). However, the Where query turns that into an IEnumerable so Count() will indeed iterate the whole enumerable in every iteration of the loop!

Instead keep track of the remaining allowances by decrementing the Allowances dictionary (or a copy if you need to keep it). Something like this (untested):

private int[] CropNumbersToBeWithinAllowances(int[] currentCombination)
{
    if (this.Allowances == null)
        return currentCombination;

    // Create a copy from the this.Allowances dictionary
    Dictionary remainingAllowances = new Dictionary(this.Allowances);

    List newCombination = new List();

    for (int i = 0; i < currentCombination.Length; i++)
    {
        int currentValue = currentCombination[i];

        if(remainingAllowances[currentValue]==0)
            continue;

        newCombination.Add(currentValue);
        remainingAllowances[currentValue]--;
    }

    return newCombination.ToArray();
}

Update

In the method with the long name you do

var sortedCombinations = (from w in combinations orderby w.Count descending select w);

int maxLength = sortedCombinations.First().Count();
for (int i = 0; i < sortedCombinations.Count(); i++)
{
    var combination = sortedCombinations.Skip(i).First();
    if (combination.Count() == maxLength)
    {
        ...

sortedCombinations is of type IEnumerable. That means that you enumerate the whole enumerable every time you call Count() and Skip() inside the loop!

You could cast the sortedCombinations enumerable to something like an array or a list and use Length or Count instead of Count() (as Frank suggested). But since you are only using the loop variable to access elements of the enumerable in order you could as well just use foreach:

var sortedCombinations = (from w in combinations orderby w.Count descending select w);

// Note: IList is an ICollection so IList.Count() will fall back to the Count property
int maxLength = sortedCombinations.First().Count();
foreach(var combination in sortedCombinations)
{
    if (combination.Count() == maxLength) // see note above
    {
        ...

StackExchange Code Review Q#54542, answer score: 5