Optimize Linq in C#

Question

I have columns list in which I need to assign Isselected as true for all except for two columns. (Bug and feature). I have used this following code to achieve it and working fine, but is there any quick or easy way to achieve the same?

DisplayColumns.ToList().ForEach(a => a.IsSelected = true);
DisplayColumns.ToList().Where(a => a.ColumnName == "Bug" ||    a.ColumnName == "Feature").ToList().ForEach(a => a.IsSelected = false);

Thanks in advance

Well, I would suggest you to use simple foreach or for loop. You are traversing collection here twice in two queries. Instead implement it by foreach loop which guarantee only single loop iteration. — Jenish Rabadiya, May 06 '15 at 06:06
First of all you don't need to call ToList() to use linq on them , it's costly and redundant . — eran otzap, May 06 '15 at 06:12

score 12 · Accepted Answer · edited May 06 '15 at 06:21

I have used this following code to achieve it and working fine, but is there any quick or easy way to achieve the same?

Well there's a cleaner way to achieve it in my view - just don't use lambdas etc at all:

foreach (var item in DisplayColumns)
{
    item.IsSelected = item.ColumnName != "Bug" && item.ColumnName != "Feature";
}

You can make the decision in one go - it's false if the column name is either "bug" or "feature"; it's true otherwise. And you don't need to call ToList and use ForEach when the C# language has a perfectly good foreach loop construct for when you want to execute some code using each item in a collection.

I love LINQ - it's fantastic - but its sweet spot is querying (hence the Q) rather than manipulation. In this case only the ToList part is even part of LINQ - List<T>.ForEach was introduced in .NET 2.0, before LINQ.

I used this code, since it looks simple and thanks for the explanation. :) — Hafiz H, May 06 '15 at 06:18

Eric · Answer 2 · 2015-05-06T06:11:00.087

6

Sure, you can assign the IsSelected at once.

DisplayColumns.ToList().ForEach(a => a.IsSelected = !(a.ColumnName == "Bug" || a.ColumnName == "Feature"));

edited May 06 '15 at 06:11

answered May 06 '15 at 06:04

Eric

5,675
16
24

StuartLC · Answer 3 · 2015-05-06T06:36:27.173

1

Provided that DisplayColumns isn't a projection of an anonymous type (in which case the properties are not re-assignable), you'll be able to change the flag in a single pass iteration through the collection.

You can also use Contains to ease the comparison. At class scope:

private static readonly string[] _searches = new [] {"Bug", "Feature"}

In your method:

DisplayColumns
.ToList() // For List.ForEach, although not @JonSkeet's caveat re mutating in Linq
.ForEach(a => a.IsSelected = !_searches.Contains(a.ColumnName));

Edit

As others have mentioned, creation of a new list simply to gain access to .ForEach to change objects in the (original) collection is wasteful and changes will be lost on a collection of value types. Rather, iterate over the original collection with foreach (or even for).

edited May 06 '15 at 06:36

answered May 06 '15 at 06:04

StuartLC

104,537
17
209
285

the where clause is wrong - `ToList`at the end of line two is wrong i guess and you miss a closing `)` there ... – Markus May 06 '15 at 06:07
Thanks - I made a bit of a hash first up. @Jehof I believe OP just wants to mutate a flag based on the value of other properties in each object. – StuartLC May 06 '15 at 06:12

eran otzap · Answer 4 · 2015-05-06T06:36:58.380

Firstly you only need to call ToList() once when creating a collection from your IEnumerable. doing this after each operator is costly and redundant.

Secondly just change your condition . all true except for the tow.

     DisplayColumns.Where(a => a.ColumnName != "Bug" && a.ColumnName != "Feature").ForEach(a => a.IsSelected = true).ToList();

Edit :

I'm sorry i like a part john's answer since this can be a re occurring thing , or IsSelected could be a Nullable , any ways lets keep it as general as possible .

I also like Stuart's approach , with the collection ( i also thought of it but didn't want to confuse . so let's give the best of all worlds.

when using linq we are actually building an expression tree at the end of which we can choose to materialize into a collection.

there for _searchs can change and each time we materialize that expression we do it with the values currently in that collection , thous making our code much more general .

 private static readonly string[] _searches = new [] {"Bug", "Feature"}

 DisplayColumns.ForEach(a => a.IsSelected =  !_searchs.Contains(a.ColumnName)).ToList();

I'm assuming ForEach is an Extension method for type IEnumrable

I believe OP used `ToList()` to obtain access to the the [`ForEach` method](http://stackoverflow.com/questions/5704969/why-foreach-linq-extension-on-list-rather-than-on-ienumerable) — StuartLC, May 06 '15 at 06:24
Is that an Extension method he wrote himself , if so it should be an Extension method of type IEnumerable and not List — eran otzap, May 06 '15 at 06:28
[I mean this](https://msdn.microsoft.com/en-us/library/bwabdf9z(v=vs.110).aspx). Its been around a long while, but as others have said, using it to perform side-effecting changes on the collection is frowned upon. — StuartLC, May 06 '15 at 06:32

MakePeaceGreatAgain · Answer 5 · 2015-05-06T06:12:10.497

0

Maybe this:

tmp = DisplayColumns.ToList();
var res = tmp.Except(tmp.Where(a => a.ColumnName == "Bug" ||    a.ColumnName == "Feature"));

foreach(var x in res) x.IsSeleceted = true;

edited May 06 '15 at 06:12

answered May 06 '15 at 06:10

MakePeaceGreatAgain

35,491
6
60
111

score 0 · Answer 6 · answered May 06 '15 at 10:16

0

Without using foreach

    DisplayColumns
    .Select(s=> {
                    s.IsSelected = (s.ColumnName == "Bug" && s.ColumnName == "Feature");
                    return s;   
                });

answered May 06 '15 at 10:16

Vishwajeet Bose

430
3
12

Optimize Linq in C#

6 Answers6