ToList().ForEach in Linq

Question

I am new to Linq.

I want to set two values in foreach statement like this

My actual code is this

foreach (Employee emp in employees)
{
    foreach(Department dept in emp.Departments)
    {
        dept.SomeProperty = null;
    }
    collection.AddRange(emp.Departments);              
}

Little refactoring turns the above into this

foreach (Employee emp in employees)
{
    emp.Departments.ToList().ForEach(u => u.SomeProperty = null))
    collection.AddRange(emp.Departments);              
}

But I want something like this

employees.ToList().Foreach(collection.AddRange(emp.Departments),
emp.Departments.ToList().ForEach(u => u.SomeProperty = null))

NO! Don't convert your collection to a list just to call that method... Use your loops. — Jeff Mercado, Oct 19 '11 at 05:15
Think about performance too while changing to Linq. Not always LINQ is helpful compared to normal loops. — Zenwalker, Oct 19 '11 at 05:17
Do that and nobody can read your code again. Self obfuscation. — Ahmet Kakıcı, Oct 19 '11 at 05:18
The original code is perfectly clear and understandable; I see no need to change it. Remember, the purpose of code is not just to communicate to the *compiler* it is to communicate to the future reader of the code; make it as clear as possible. — Eric Lippert, Oct 19 '11 at 14:30
@EricLippert Thank you Eric. Had I not been accustomed to READING all comments now I would have missed this valuable recommendation. — Stephen Patten, Mar 14 '13 at 17:21

score 85 · Accepted Answer · edited Jan 07 '20 at 10:53

85

You shouldn't use ForEach in that way. Read Lippert's “foreach” vs “ForEach”

If you want to be cruel with yourself (and the world), at least don't create useless List

employees.All(p => {
    collection.AddRange(p.Departments);
    p.Departments.All(u => { u.SomeProperty = null; return true; } );
    return true;
});

Note that the result of the All expression is a bool value that we are discarding (we are using it only because it "cycles" all the elements)

I'll repeat. You shouldn't use ForEach to change objects. LINQ should be used in a "functional" way (you can create new objects but you can't change old objects nor you can create side-effects). And what you are writing is creating so many useless List only to gain two lines of code...

edited Jan 07 '20 at 10:53

openshac

4,966
5
46
77

answered Oct 19 '11 at 05:17

xanatos

109,618
12
197
280

23

Although what we're suggesting here is to replace the `ToList()` abuse with an `All` abuse. ;) – Ilian Oct 19 '11 at 05:32
2

@IlianPinzon I thought I was quite clear of this. Do you think I should **bold** it? :-) – xanatos Oct 19 '11 at 05:33
1

I prefer a blinking marquee that says "DO NOT (AB)USE." :) Newbies may read the example as gospel if they skip the disclaimer text above. :) – Ilian Oct 19 '11 at 05:38
1

Atleast with removing the ToList it isn't copying the collection in memory just to iterate over it and throw it away. On large-ish collection ToList is deadly. – Phill Oct 19 '11 at 05:44

Charles Graham · Answer 2 · 2011-10-19T05:35:05.363

24

As xanatos said, this is a misuse of ForEach.

If you are going to use linq to handle this, I would do it like this:

var departments = employees.SelectMany(x => x.Departments);
foreach (var item in departments)
{
    item.SomeProperty = null;
}
collection.AddRange(departments);

However, the Loop approach is more readable and therefore more maintainable.

edited Oct 19 '11 at 05:35

answered Oct 19 '11 at 05:21

Charles Graham

1,157
5
6

1

+1 Wow! Beautiful use of `SelectMany` (not that I would use it :-) ) – xanatos Oct 19 '11 at 05:24
Though it might be better to add the range after you modify them so it logically makes sense that you're adding modified items to the collection. The collection may even store copies of the items so this might not work. – Jeff Mercado Oct 19 '11 at 05:31
4

It's probably even better to remove the `AddRange` line which does an unnecessary additional traversal of the sequence. One can do `collection.Add(item)` inside the existing loop with the same result. – Ilian Oct 19 '11 at 05:35
@Jeff I hadn't thought of that, fixed it. – Charles Graham Oct 19 '11 at 05:35

score 14 · Answer 3 · edited Aug 03 '17 at 09:07

14

employees.ToList().ForEach(
     emp=>
     {
          collection.AddRange(emp.Departments);
          emp.Departments.ToList().ForEach(u=>u.SomeProperty = null);
     });

edited Aug 03 '17 at 09:07

TheBoubou

19,487
54
148
236

answered Oct 19 '11 at 05:15

artwl

3,502
6
38
53

1

So adding .ToList() allows Linq .ForEach()... Nice! – Gerardo Grignoli Aug 21 '19 at 21:04

score 5 · Answer 4 · answered Oct 19 '11 at 05:25

5

Try this:

foreach (var dept in employees.SelectMany(e => e.Departments))
{
   dept.SomeProperty = null;
   collection.Add(dept);
}

answered Oct 19 '11 at 05:25

Ilian

5,113
1
32
41

score 1 · Answer 5 · edited Nov 25 '14 at 09:06

1

employees.ToList().Foreach(u=> { u.SomeProperty = null; u.OtherProperty = null; });

Notice that I used semicolons after each set statement that is -->

u.SomeProperty = null;
u.OtherProperty = null;

I hope this will definitely solve your problem.

edited Nov 25 '14 at 09:06

Ilija Dimov

5,221
7
35
42

answered Nov 25 '14 at 08:46

Saurabh

47
1
8

score 0 · Answer 6 · edited Sep 01 '20 at 15:53

0

You can use Array.ForEach()

Array.ForEach(employees, employee => {
   Array.ForEach(employee.Departments, department => department.SomeProperty = null);
   Collection.AddRange(employee.Departments);
});

edited Sep 01 '20 at 15:53

Neuron

5,141
5
38
59

answered Apr 16 '20 at 10:12

maeneak

573
6
10

score -3 · Answer 7 · answered Oct 19 '11 at 05:22

-3

you want this?

    employees.ForEach(emp =>
    {
        collection.AddRange(emp.Departments.Where(dept => { dept.SomeProperty = null; return true; }));
    });

answered Oct 19 '11 at 05:22

ojlovecd

4,812
1
20
22

S2S2 · Answer 8 · 2019-11-05T11:19:27.543

-4

Try with this combination of Lambda expressions:

employees.ToList().ForEach(emp => 
{
    collection.AddRange(emp.Departments);
    emp.Departments.ToList().ForEach(dept => dept.SomeProperty = null);                    
});

edited Nov 05 '19 at 11:19

answered Oct 19 '11 at 05:22

S2S2

8,322
5
37
65

ToList().ForEach in Linq

8 Answers8

Linked