21

I have the following query:

drivers.Select(d => { d.id = 0; d.updated = DateTime.Now; return d; }).ToList();

drivers is a List which comes in with different id's and updated values, so I am changing the values in the Select, but is the proper way to do it. I already know that I am not reassigning drivers to drivers because Resharper complains about it, so I guess it would be better if it was:

drivers = drivers.Select(d => { d.id = 0; d.updated = DateTime.Now; return d; }).ToList();

but is this still the way someone should assign new values to each element in the drivers List?

Xaisoft
  • 45,655
  • 87
  • 279
  • 432
  • possible duplicate of [Linq side effects](http://stackoverflow.com/questions/5632222/linq-side-effects) – nawfal May 17 '13 at 20:09
  • another possible duplicate https://stackoverflow.com/questions/8127430/linq-to-objects-join-two-collections-to-set-values-in-the-first-collection/62326244 – Marisol Gutiérrez Jun 11 '20 at 14:08

3 Answers3

38

Although this looks innocent, especially in combination with a ToList call that executes the code immediately, I would definitely stay away from modifying anything as part of a query: the trick is so unusual that it would trip up readers of your program, even experienced ones, especially if they never saw this before.

There's nothing wrong with foreach loops - the fact that you can do it with LINQ does not mean that you should be doing it.

Sergey Kalinichenko
  • 714,442
  • 84
  • 1,110
  • 1,523
  • 1
    I would use `ForEach` for single line statement, not for something that requires curly braces (multiple statements). `foreach` is indeed better here. – nawfal May 16 '13 at 18:17
  • You are correct, it tripped me up :) I agree with your second statement as well. – Xaisoft May 16 '13 at 18:18
  • @nawful, many people find state modification in lambdas to be somewhat distasteful. – Kirk Woll May 16 '13 at 18:18
  • 2
    But if I did use a ForEach, would that be the better alternative as far as readability? – Xaisoft May 16 '13 at 18:19
  • 1
    @Xaisoft `ForEach` could work better, but it is not part of LINQ: it is defined on `List`, not on `IEnumerable`, so I am nearly certain that it was built with the idea of modification in mind (as long as the list itself stays intact, of course). Using a `foreach` loop vs. `List.ForEach` is a question of personal preference. – Sergey Kalinichenko May 16 '13 at 18:25
  • @KirkWoll I agree with that if you mean about `Linq`. But I do not know why would with `ForEach` on `List`. Too much `Linq` love these days have led to despising age old mutating `ForEach` even though the former is not about `Linq` or query. Btw, tag my name properly, so that I'm notified :) – nawfal May 16 '13 at 18:54
  • @nawfal, sorry about the typo. There is disagreement over whether the `ForEach` method should even exist on `List`. (Mr. Lippert below has offered [his thoughts on the subject before](http://blogs.msdn.com/b/ericlippert/archive/2009/05/18/foreach-vs-foreach.aspx)). – Kirk Woll May 16 '13 at 19:37
  • @KirkWoll I have read that. More like philosophical differences. Never mind, to each his own :) – nawfal May 16 '13 at 19:40
33

NEVER DO THIS. A query should be a query; it should be non-destructively asking questions of a data source. If you want to cause a side effect then use a foreach loop; that's what it's for. Use the right tool for the job.

Eric Lippert
  • 647,829
  • 179
  • 1,238
  • 2,067
  • 5
    Thank you Eric. The only way to know the right way is to know you are doing it wrong in the first place :) – Xaisoft May 16 '13 at 18:29
9

Ok I will make an answer myself.

Xaisoft, Linq queries, be it lambda expression or query expression, shouldn't be used to mutate list. Hence your Select 

drivers = drivers.Select(d => { d.id = 0; d.updated = DateTime.Now; return d; }).ToList();

is bad style. It confuses/unreadable, not standard, and against Linq philosophy. Another poor style of achieving the end result is:

drivers.Any(d => { d.id = 0; d.updated = DateTime.Now; return false; });

But that's not to say ForEach on List<T> is inappropriate. It finds uses in cases like yours, but do not mix mutation with Linq query, thats all. I prefer to write something like: 

drivers.ForEach(d => d.updated = DateTime.Now);

Its elegant and understandable. Since it doesn't deal with Linq, its not confusing too. I don't like that syntax for multiple statements (as in your case) inside the lambda. It's a little less readable and harder to debug when things get complex. In your case I prefer a straight foreach loop.

foreach (var d in drivers)
{ 
    d.id = 0; 
    d.updated = DateTime.Now; 
}

Personally I like ForEach on IEnumerable<T> as a terminating call to Linq expression (ie, if the assignment is not meant to be a query but an execution).

Community
  • 1
  • 1
nawfal
  • 70,104
  • 56
  • 326
  • 368
  • @Xaisoft Actually the thing I hate about `ForEach` on `List` is the fact they appear by default. I don't think its fit as a framework level construct. I prefer such higher level constructs coming in at user's discretion, as extension or so. If tailor-made stuffs helps the end user, well go for it. There is a lot geeky going in there and its up to individuals to select if its production fit or not. [Here is another `switch-case` implemented for `Type`,](http://stackoverflow.com/a/1426626/661933) well its awesome. Embrace it or turn away. Don't bitch about it.. – nawfal May 17 '13 at 09:48
  • @Xaisoft whoever does, din mean anyone in particular :) – nawfal May 17 '13 at 12:43
  • One of the problems is that maybe when you are given too many options, you don't know what to do. Part of that comes with experience and knowing what the right tool for the job is as Eric Lippert pointed out. – Xaisoft May 17 '13 at 12:45
  • 1
    @Xaisoft I agree, and your question is relevant too. – nawfal May 17 '13 at 12:50