Building a LINQ query programmatically without local variables tricking me

Question

Assume my objects are in perfect working order (i.e. TDD makes me think they work).

I have a list that I create like this (except indented properly):

var result = from v in vendors
             from p in v.Products
             orderby p.Name
             select p;

This works - I get all products from all vendors.

Now I have a list of conditions, built up at runtime by the user. Let's apply them:

foreach (Attribute a in requiredAttributes)
{
    result = result.Where(p => p.Attributes.Contains(a));
}

This may be primitive, but I thought it'd work. However, after this foreach loop is finished, when you enumerate "result" it will contain all products that has the LAST Attribute of the requiredAttributes collection in it's Attributes property (also a collection).

To me, this smells like "a" is overwritten somewhere with each trip trough the loop, and only the last one applies.

Short of somehow writing an extension method to IEnumerable called ContainsAll(IEnumerable) or something to that effect, how can I achieve what I want, which is basically a logical AND, giving me only those products that has ALL the required attributes?

Thanks guys - I am giving the "accepted answer" to Jon as it seems it might be more suitable for the "general" case. But I will definately check out Omer's suggestion as well, didn't know you could do that. Votes to all. :) — Rune Jacobsen
– Rune Jacobsen, Commented Oct 10, 2008 at 6:30
The new way to fix this issue is to just use C#5. See also Has foreach's use of variables been changed in C# 5? — Brian
– Brian, Commented Jan 15, 2016 at 14:38

Jon Skeet · Accepted Answer · 2008-10-10 05:48:31Z

20

(Edited for clarity.)

The problem is the foreach loop, and the fact that the "a" variable is being captured and then changed each time. Here's a modification which will work, by effectively introducing a "new" variable for each iteration of the loop, and capturing that new variable.

foreach (Attribute a in requiredAttributes)
{
    Attribute copy = a;
    result = result.Where(p => p.Attributes.Contains(copy));
}

Omer's solution is a cleaner one if you can use it, but this may help if your real code is actually more complicated :)

EDIT: There's more about the issue in this closures article - scroll down to "Comparing capture strategies: complexity vs power".

edited Oct 10, 2008 at 5:48

answered Oct 10, 2008 at 5:41

Jon Skeet

1.5m893 gold badges9.3k silver badges9.3k bronze badges

Sign up to request clarification or add additional context in comments.

1 Comment

Grank Over a year ago

I learned this critical nuance from ReSharper's puzzling warnings about it ;)

Omer van Kloeten · Accepted Answer · 2008-10-10 05:31:22Z

7

var result = from v in vendors
             from p in v.Products
             where requiredAttributes.All(a => p.Attributes.Contains(a))
             orderby p.Name
             select p;

HTH.

answered Oct 10, 2008 at 5:31

Omer van Kloeten

12k9 gold badges45 silver badges54 bronze badges

Comments

Brian · Accepted Answer · 2008-10-10 05:43:52Z

5

I haven't coded it up, but change

foreach (Attribute a in requiredAttributes){    
    result = result.Where(p => p.Attributes.Contains(a));
}

to

foreach (Attribute a in requiredAttributes){    
    Attribute b = a;
    result = result.Where(p => p.Attributes.Contains(b));
}

should fix it too, I think.

answered Oct 10, 2008 at 5:43

Brian

119k18 gold badges246 silver badges304 bronze badges

Collectives™ on Stack Overflow

Building a LINQ query programmatically without local variables tricking me

3 Answers 3

1 Comment

Comments

Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

1 Comment

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related