Assigning values inside a LINQ Select?

Question

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?

Answer 1

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.

Answer 2

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).

Answer 3

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.