Should I call ToList() inside foreach C#

Question

Is this code normal?

Should I call ToList(). If yes why?

It is an entity framework. Or It doesn't matter and I may not call to list?

var followers = from user in project.Followers
                where followersIds.Contains(user.Id)
                select user;

foreach (var follower in followers.ToList())
{
     project.Followers.Remove(follower);
}

You modify the underlying collection inside the body of `foreach`. — Jeppe Stig Nielsen, Apr 07 '20 at 15:49
@JeppeStigNielsen that depends on a lot of things; it *could be*, but the query could also be independent once execution has started, especially for `IQueryable`; it will be implementation specific, but yes this is a definite risk — Marc Gravell, Apr 07 '20 at 15:51
Anytime you're removing elements from a collection inside a `foreach`, you should probably call `ToList()` on that collection. Even if the original collection was a list. That's a popular workaround to avoid running into `InvalidOperationException`s. — 41686d6564 stands w. Palestine, Apr 07 '20 at 15:52
@MarcGravell I agree. I was careful not to conclude whether or not the `.ToList()` was necessary, because I do not know how it would be implemented in Entity Framework. — Jeppe Stig Nielsen, Apr 07 '20 at 15:54
Note that this will generate a lot of SQL statements if there are a lot of followers. You may be interested in [bulk delete in entity framework](https://stackoverflow.com/q/11592176/215552) — Heretic Monkey, Apr 07 '20 at 15:58

score 2 · Answer 1 · answered Apr 07 '20 at 15:52

2

The ToList is there to avoid running into a collection modified while iterating runtime error. ToList() makes an eager copy of the collection and then you iterate over it removing items form the original source project.Followers.

It looks kind of hacky but that is the reason why ToList is there.

answered Apr 07 '20 at 15:52

InBetween

32,319
3
50
90

__If__ this had been Linq-to-Objects, and the underlying source was, say, a `Dictionary<,>` in memory, then `.ToList()` _would_ be necessary because a `Dictionary<,>` produces an `IEnumarator<>` which will keep track of whether the `Dictionary<,>` was modified during the lifetime of the enumerator, and throw an exception in that case. However, this Linq is to Entity Framework. Do you know that their implementation also leads to that "collection modified" runtime error? – Jeppe Stig Nielsen Apr 07 '20 at 15:59
@JeppeStigNielsen No, as a matter of fact I don't, so that is a very good point. The reason why its there is evidently because whoever wrote the code thought it was needed to avoid the described issue. Wether that belief is true, I can't say. I'll update answer. – InBetween Apr 07 '20 at 16:35

score 1 · Answer 2 · answered Apr 07 '20 at 16:15

Well, assuming you are using Entity Framework (from tags).
When you execute the Linq expression you get an IQueryable collection. This IQueryable is not a truthy collection stored in memory with all the records fetched from the DB, it's kinda a statement where to query to the database server. I mean, You will not query the database since this IQueryable is accessed.

The IQueryable collection can be accessed through iteration or collection casting (such as IList, Array, ...).

/// This will yield item by item trough the loop.
IQueryable followers = /* linq expression */
foreach (var follower in followers)
{
   // ...
}

The chunk above has a tiny better performance.

IQueryable followers = /* linq expression */
IList list = followers.ToList(); // At this point, the query is executed in the database server
foreach (var follower in list)
{
   // ...
}

While this apporach is more safe.
It is a common practice to cast an IQueryable to an IList or similar to prevent concurrency issues, thread troubles, etc.

The important thing is to "seek" the IQueryable once all the actions on the query are done (filtering, sorting, ...).

Having an IQueryable like so

IQueryable followers = project.Followers;

you can now add actions to the IQueryable with no memory or performance cost.

followers = followers.Where(/* Any Func<Type, bool> action here*/);
followers = followers.OrderBy(/* Property KeySelector... */);

Once all the query is built, the query can be executed.

IList realDataInMemory = followers.ToList();

If you call the #ToList method at the first step.

IList followers = project.Followers.ToList();

the next Linq expressions will be executed to the in-memory collection.

All sounds correct. But can we answer what happens in the example from the question if the `.ToList()` is removed? Entity Framework. — Jeppe Stig Nielsen, Apr 07 '20 at 16:47
You are right, I missed the point of the question. Well, I will add then, calling `ToList()` to the iterable collection is a **must** to avoid a runtime exception, when talking about Entity Framework plus Linq, the query should be executed before, then call `ToList()` from the already built `IList`. — VRoxa, Apr 07 '20 at 17:12
I'm not a native speaker. So, for me, it looks no difference for performance in: 1. create Ilist variable and put in loop 2. call ToList directly in loop. Right? — Mediator, Apr 08 '20 at 06:33

Should I call ToList() inside foreach C#

2 Answers2