32

What is the preferable way for transferring some items (not all) from one list to another.

What I am doing is the following:

var selected = from item in items
               where item.something > 10
               select item;

otherList.AddRange(selected);

items.RemoveAll(item => selected.Contains(item));

In the interest of having the fastest/best code there is, is there a better way?

Sinan Ünür
  • 116,958
  • 15
  • 196
  • 339
Stécy
  • 11,951
  • 16
  • 64
  • 89
  • I would look at using the ForEach method on List to handle this. Also, you probably want to standardize on using either the query syntax or method syntax, not both. – Jeffrey Lott Jun 22 '09 at 20:47

5 Answers5

22

I suggest:

var selected = items.Where(item => item.Something > 10).ToList();
items = items.Except(selected).ToList();
otherList.AddRange(selected);
Mehrdad Afshari
  • 414,610
  • 91
  • 852
  • 789
20

I'd try @Mehrdad's answer, and maybe test it against this one too...

var selected = items.Where(item => item.Something > 10).ToList();
selected.ForEach(item => items.Remove(item));
otherList.AddRange(selected);
Scott Ivey
  • 40,768
  • 21
  • 80
  • 118
  • 1
    Got this error doing it this way: InvalidOperationException: Collection was modified; enumeration operation may not execute. – tclark333 Apr 14 '21 at 17:31
7

RemoveAll goes trough each item and enumerates all values of your selected list every time. This will take longer than it should...

What I'd do is put the condition directly in the RemoveAll parameter:

items.RemoveAll(item => item.something > 10);

If you do this and don't change the rest of your code there would be code duplication, which is not good. I'd do the following to avoid it:

Func<ItemType, bool> selectedCondition = (item => item.something > 10);

otherList.AddRange(items.Where(selectedCondition));

items.RemoveAll(new Predicate<ItemType>(selectedCondition));
Meta-Knight
  • 17,626
  • 1
  • 48
  • 58
  • +1. This is the best method (unless the Where condition is much more complex than checking element equality). Just make sure you are not changing the collection between the method calls. You might lose elements. – Mehrdad Afshari Jun 22 '09 at 21:03
  • btw, you can't use `var` with a lambda (even if you fix the broken syntax). – Marc Gravell Jun 22 '09 at 21:09
  • I think it's fixed now. Was there any other "broken" syntax besides my missing semicolon? – Meta-Knight Jun 22 '09 at 21:39
  • 1
    Seeing as how this has been here for six years and has 4 plus votes, I have a bad feeling that I must be doing something wrong. But I can't get the compiler to accept the RemoveAll(selectedCondition) method invocation. It keeps saying it wants a Predicate, not a Func. In order to get it to work I had to use the techniques described here: http://stackoverflow.com/questions/731249/how-to-convert-funct-bool-to-predicatet – RenniePet Apr 10 '15 at 00:03
  • 1
    @RenniePet: You're right, RemoveAll seems to accept only predicates. I edited the answer. – Meta-Knight Apr 10 '15 at 12:43
6

That is quite bad performance wise - it actually enumerates a query n times (for n items in items). It would be better if you built (for example) a HashSet<T> of the items to manipulate.

To give a simple example just with int values:

    var items = new List<int> { 1, 2, 3, 4, 5, 6 };
    var otherList = new List<int>();
    var selected = new HashSet<int>(items.Where(
        item => item > 3));
    otherList.AddRange(selected);
    items.RemoveAll(selected.Contains);
Marc Gravell
  • 1,026,079
  • 266
  • 2,566
  • 2,900
  • I'm not sure if I correctly see how the query will enumerate n times... Is it because of the use of the "selected" query in both the AddRange and RemoveAll? At worst, I thought that the enumeration would go only twice... – Stécy Jun 22 '09 at 20:53
  • 2
    "selected" is an IEnumerable<> query - *not* a container. "selected.Contains" enumerates this query each time it is invoked. – Marc Gravell Jun 22 '09 at 20:55
  • Could I use another list instead of a HashSet? I fail to see why it would be better... – Stécy Jun 22 '09 at 20:58
  • 1
    Stecy: Lookup is O(n) in a list while it can be O(1) in a HashSet. Yet, I believe from the readability perspective, an Except method which uses a HashSet internally works better. – Mehrdad Afshari Jun 22 '09 at 21:00
1

How about a partition:

int[] items = { 5, 4, 1, 3, 9, 8, 6, 7, 2, 0 };
var partition = items.ToLookup(x => x > 5);
var part1 = partition[true];
var part2 = partition[false];
Undo
  • 25,519
  • 37
  • 106
  • 129
Ray
  • 2,974
  • 20
  • 26