Why I need to enumerate my list inside the program's logic for it to work in this case?

Question

I have this simple piece of code:

IEnumerable<AccessData> reports = _model.GetAllAccessEvents();
foreach (var ap in AccessPoints.Where(x => !x.IsChecked))
    reports = reports.Where(x => x.AccessPointId != ap.Id);

It should remove the non checked objects from the reports IEnumerable. But it doesn't work (it returns the complete list). Unless I do this:

foreach (var ap in AccessPoints.Where(x => !x.IsChecked))
    reports = reports.Where(x => x.AccessPointId != ap.Id).**ToList()**;

Then the results come properly filtered. Why do I need to enumerate my results in the middle of the logic for it to work?

Answer 1

Technically, you do not need to enumerate your data in a loop in order to make it work (although it is one of the ways to fix the problem). The reason why it does not work without the ToList is that the code in the inner Where refers to a modified closure - variable ap which is modified in a loop.

Using a temporary variable for it would fix the problem:

IEnumerable<AccessData> reports = _model.GetAllAccessEvents();
foreach (var ap in AccessPoints.Where(x => !x.IsChecked)) {
    var theId = ap.Id; // Introduce a temporary variable
    reports = reports.Where(x => x.AccessPointId != theId);
}

Another solution would be using a Where that compares AccessPointIds to all items in the access events without a loop, like this:

reports = _model
    .GetAllAccessEvents()
    // The condition ensures that x is such that its ID is not present
    // among the AccessPointIds of the checked access points:
    .Where(x => !AccessPoints.Any(ap => ap.IsChecked && ap.AccessPointId == x.Id));

Answer 2

What version of C# are you using? The behavior of a loop variable in a closure changed with C# 5 (if I recall correctly). If this is indeed your problem, then the following should work, too:

IEnumerable<AccessData> reports = _model.GetAllAccessEvents();
foreach (var ap in AccessPoints.Where(x => !x.IsChecked))
{
    var closureAp = ap;
    reports = reports.Where(x => x.AccessPointId != closureAp.Id);
}

More efficent, perhaps, though less concise (assumes the type of Id is int; change as needed):

var reports = _model.GetAllAccessEvents().ToList();
var idsToRemove = new HashSet<int>(AccessPoints.Where(x => !x.IsChecked).Select(x => x.Id));
reports.RemoveAll(report -> idsToRemove.Contains(report.AccessPointId));

or

var reports =
    from accessPoint in AccessPoints
    where !accessPoint.IsChecked
    join accessEvent in _model.GetAllAccessEvents()
        on accessPoint.Id equals accessEvent.AccessPointId
    select accessEvent;

Note that using Join or HashSet will give the algorithm better time complexity, meaning that its performance will deteriorate less rapidly as the number of elements grows.