CODEWITHSUNDEEP
c#.netlinqlambda
BVernon
BVernon

Reputation: 3747

How to fix this lambda expression?

I have this function for sorting by multiple user selected columns:

public override void Sort(SortFields sortFields)
{
    if (sortFields == null || sortFields.Count() == 0) return;

    var res = FilteredItems.OrderBy(o => o[sortFields[0].Name], sortFields[0].Ascending ? comparer : comparerDesc);

    for (int x = 1; x < sortFields.Count(); x++)
        res = res.ThenBy(o => o[sortFields[x].Name], sortFields[x].Ascending ? comparer : comparerDesc);

    FilteredItems = new BindingList<T>(res.ToList());

    if (ListChanged != null)
        ListChanged(this, new ListChangedEventArgs(ListChangedType.Reset, null));
}

The problem, of course, is that when res.ToList() is called x has been incremented to one greater than the highest index in the list and it throws an error. I know I could go ahead and do a ToList() after each sort but that defeats the purpose and isn't even guaranteed to sort correctly with all linq providers. I'm sure there's a simple solution to this... anyone want to tell me what it is? :)

Upvotes: 1

Views: 85

Answers (2)

Grant Winney
Grant Winney

Reputation: 66439

It looks like you may be getting tripped up by a closure over the variable x.

That single variable is incremented on every iteration of the loop. It's incremented one last time, at which point it's 1 greater than the number of items in "sortFields", your conditional statement evaluates to False, and your for loop ends.

As user2864740 pointed out, and Eric states in his article:

Closures close over variables, not over values.

So when you then call ToList(), your chained ThenBy statements retain the final value of the variable x, which was 1 greater than the highest index.

Instead, assign the incrementer to an intermediary variable inside the loop. When you call ToList(), the final value of x won't matter.

for (int x = 1; x < sortFields.Count(); x++)
{
    int y = x;
    res = res.ThenBy(o => o[sortFields[y].Name], sortFields[y].Ascending ? comparer : comparerDesc);
}

Also from Eric's article, this is (hopefully) being corrected soon, though only in foreach loops apparently, not for loops:

In C# 5, the loop variable of a foreach will be logically inside the loop, and therefore closures will close over a fresh copy of the variable each time. The "for" loop will not be changed.

Upvotes: 4

Arijit Mukherjee
Arijit Mukherjee

Reputation: 3875

Try

var res = FilteredItems.OrderBy(o => o[sortFields[0].Name], sortFields[0].Ascending ? comparer : comparerDesc).ToList();

Upvotes: 0

Related Questions