CODEWITHSUNDEEP
c#linq
Anders Arpi
Anders Arpi

Reputation: 8417

Looping LINQ queries on the same IEnumerable - what am I missing?

Using a Dictionary I'm looping over an IEnumerable and apply filters according to the filter dictionary each time. FilterType enum decides what field the filter should be applied to and the string is the freetext filter itself.

The problem I have is that if I add more than one filter to the dictionary (i.e. both a FilterType.Customer and a FilterType.Name filter) the filters will be applied to both the Customer and Name fields BUT the freetext filter will be taken from the first filter added. I.e. in the code below the nameFilterBox.Text gets applied to both the Name and Customer fields in the LINQ query, whereas customerFilterBox.Text doesn't get applied at all. So the FilterType part of the dictionary gets read twice but the string filter only gets read from the first one.

I really can't understand why this is happening.

static IEnumerable<Jobb> jobQuery = GetInitialJobQuery();

Dictionary<FilterType, string> filters = new Dictionary<FilterType, string>();
filters.Add(FilterType.Name, nameFilterBox.Text);
filters.Add(FilterType.Customer, customerFilterBox.Text);

foreach (var filter in filters)
{
    switch (filter.Key)
    {
        case FilterType.Customer:
            jobQuery = jobQuery.Where(x => x.KUNDREF != null &&
                       x.KUNDREF.ToLower().Contains(filter.Value.ToLower()));
            break;

        case FilterType.Name:
            jobQuery = jobQuery.Where(x => x.JOBBESKR != null &&
                       x.JOBBESKR.ToLower().Contains(filter.Value.ToLower()));
            break;
    }
}

Upvotes: 1

Views: 361

Answers (2)

Timwi
Timwi

Reputation: 66604

This is the foreach loop variable capturing bug. Your lambdas operate on the last value in filters, not the one you think.

The workaround is to declare a new variable inside the foreach loop:

foreach (var filterForeach in filters)
{
    var filter = filterForeach;  // for the lambdas
    switch (filter.Key)
    {
        case FilterType.Customer:
            jobQuery = jobQuery.Where(x => x.KUNDREF != null &&
                       x.KUNDREF.ToLower().Contains(filter.Value.ToLower()));
            break;

        case FilterType.Name:
            jobQuery = jobQuery.Where(x => x.JOBBESKR != null &&
                       x.JOBBESKR.ToLower().Contains(filter.Value.ToLower()));
            break;
    }
}

Alternatively, since you use only filter.Value inside the lambdas, you could do this too:

foreach (var filter in filters)
{
    var value = filter.Value.ToLower();  // for the lambdas
    switch (filter.Key)
    {
        case FilterType.Customer:
            jobQuery = jobQuery.Where(x => x.KUNDREF != null &&
                       x.KUNDREF.ToLower().Contains(value));
            break;

        case FilterType.Name:
            jobQuery = jobQuery.Where(x => x.JOBBESKR != null &&
                       x.JOBBESKR.ToLower().Contains(value));
            break;
    }
}

Eric Lippert blogged about this here: Closing over the loop variable considered harmful.

Upvotes: 3

Guffa
Guffa

Reputation: 700690

I'm not sure exactly, but I think that it has something to do with how the value is read from the enumerator, so that it doesn't end up in the closure for the lambda. As the values are used after the loop, you will be using the value left in the Current property of the enumerator, i.e. the last item.

Try copying the value into a local variable, to make it end up in the closure:

string value = filter.Value.ToLower();
switch (filter.Key) {
  case FilterType.Customer:
    jobQuery = jobQuery
      .Where(x => x.KUNDREF != null && x.KUNDREF.ToLower().Contains(value));
    break;
case FilterType.Name: {
  jobQuery = jobQuery
    .Where(x => x.JOBBESKR != null && x.JOBBESKR.ToLower().Contains(value));
  break;
}

Upvotes: 1

Related Questions