Is it better to do ToList() before ToDictionary?

Question

Is it worth to do the ToList() before doing the GroupBy() and ToDictionary() twice as in example below. Does the ToList() may maximize the performance when creating the dictionary? Without ToList() Resharper is yelling about possible multiple enumeration.

public void SomeMethod(IEnumerable<oldItem> oldItems)
{
    var collection = oldItems.Select(i => new item()).ToList();
    var dict1 = collection.ToDictionary(i => i.Key);
    var dict2 = collection
         .GroupBy(i => i.FieldA)
         .ToDictionary(g => g.Key, g => new Flags(g.ToArray));
}

What makes you think the resharper warning has anything to do with performance? — Mixxiphoid, May 12 '17 at 10:26
You do iterate over the collection `collection` 2 times so in this case it would make sense to have `ToList()` at the end of the first line. — Igor, May 12 '17 at 10:27
Answer to title: `No`, answer to actual question(_"worth to do the ToList() before doing the GroupBy() and ToDictionary() **twice**"_): `Maybe` (depends on the type of the sequence, could be a query that takes hours to execute). — Tim Schmelter, May 12 '17 at 10:28
All `ToList` will do is create a `List` out of the previous `IEnumerable`'s contents. The following LINQ methods will execute in exactly the same way, so the only thing you will have done is allocate memory for an arbitrary list and force your LINQ to iterate over the entire collection an additional time for no reason. So no, it will not help your performance. — Abion47, May 12 '17 at 10:29
@Mixxiphoid: I am not saying it has something with performance. It is just annoying. — jotbek, May 12 '17 at 10:36
@Mixxiphoid Depending on the source of the enumerable, it most definitely *can* have something to do with performance. — Rob, May 12 '17 at 10:42
@LeiYang: you uninstall every warning system that annoys you? Better fix the reasons for the warning. — Tim Schmelter, May 12 '17 at 10:42
@Rob sure, but that is not the point of my comment. Resharper won't give that warning because a performance hit is happening. — Mixxiphoid, May 12 '17 at 10:55
Possible duplicate of [Handling warning for possible multiple enumeration of IEnumerable](http://stackoverflow.com/questions/8240844/handling-warning-for-possible-multiple-enumeration-of-ienumerable) — Mixxiphoid, May 12 '17 at 10:56

Tim Schmelter · Accepted Answer · 2017-05-12T10:56:24.157

Is it better to do ToList before ToDictionary?

No, Enumerable.ToDictionary enumerates all items anyway so there is no benefit. The opposite is true, you need to fill another collection in a loop for no reason

Is it better to do ToList() before ToDictionary if i need multiple dictionaries?

Probably. It depends on the type of the sequence. It could be a database query that takes ages to execute. Without ToList you will execute it multiple times. But it could also be a collection or very cheap query. Resharper wants you to think about it.

There's another subtle difference which has nothing to do with performance. If you don't store the sequence in a collection(f.e with ToList) you could be in a deferred execution context(f.e. if oldItems is a database query). That means whenever you execute this query you will get the current result which could be different to the previous execution's result. That might be desired, you just have to keep that in mind.

It seems like a clear answer. Exactly Resharper made me to think about it :). Thanks — jotbek, May 12 '17 at 10:43

Mixxiphoid · Answer 2 · 2017-05-12T10:36:19.433

1

Resharper warns you that the next item of the IEnumerable may be consumed twice when you intend only once. Putting the items in a list consumes the IEnumerable into a list. You can enumerate over a list as many times you want, where IEnumerable will yield the next item. collection may be consumed by the first usages, therefore the behaviour may be unexpected for the second use.

edited May 12 '17 at 10:36

answered May 12 '17 at 10:30

Mixxiphoid

1,044
6
26
46

score 0 · Answer 3 · answered May 12 '17 at 10:37

0

Without the ToList your collection is a SelectEnumerator around the oldItems, which will be enumerated twice for both dict1 and dict2. This means that i => new item() will be called twice for each elements of oldItems.

answered May 12 '17 at 10:37

György Kőszeg

17,093
6
37
65

score 0 · Answer 4 · answered May 12 '17 at 13:25

I would state it simply - do not enumerate IEnumerable multiple times. Just do not do that. If you need to iterate over multiple times, you can define IList/IReadOnlyList interface. Btw. you can construct the second dictionary from values of the first in this case.

Is it better to do ToList() before ToDictionary?

4 Answers4