Setting value of object property in List using LINQ

Question

I have a List<Email>() and my Email object looks like this:

public class Email
{
   public string EmailAddress { get; set; }
   public bool IsPrimary { get; set; }
}

When I add a new email address that is set as primary, I want to set all the others as non-primary. I currently handle this using a foreach. Can I handle this using LINQ?

My current code is:

foreach (var item in emails)
{
   if(item.EmailAddress.ToLower() != newEmailAddress.ToLower() && item.IsPrimary)
      item.IsPrimary = false;
}

Please see https://blogs.msdn.microsoft.com/ericlippert/2009/05/18/foreach-vs-foreach/ — Peter B, Mar 02 '18 at 22:12
LINQ is intended for querying and not modification. Having said that, there is a `List.ForEach` operator, but not much increase in clarity. — NetMage, Mar 02 '18 at 22:13
Just use a `foreach`. LINQ is not for modification. Its not LINM, its LINQ. Dont use a square peg in a round hole. All your current answers are _bad_ answers. — maccettura, Mar 02 '18 at 22:30
The major red flag is: update all other instances.... bad design in my opinion. You should have a single mention of DefaultEmailId in a class, and IsDefault should be a an on-request check against that. At least in the in-memory model... So many years down the line and people still think models and code should map 1:1 to a database (sadface) — Alexandru Clonțea, Mar 02 '18 at 22:33
As an aside, with a data structure like this, you're leaving yourself open to having some weird data, like multiple primary addresses. You're much better off storing the ID of the primary against another object (e.g. `user.PrimaryEmailId`), that way a single update will do the job. — DavidG, Mar 02 '18 at 22:34

McAden · Accepted Answer · 2018-03-02T23:01:59.803

1

Linq queries collections, it doesn't modify them. The only spot in this equation that linq would come into play is actually making it a part of the enumeration - filtering the collection you're iterating over rather than doing an if statement inside it.

foreach (var item in emails.Where(e => e.IsPrimary && !e.EmailAddress.Equals(newEmailAddress, StringComparison.InvariantCultureIgnoreCase)))
{
      item.IsPrimary = false;
}

EDIT: I didn't originally include it as it's not LINQ and that's what the question is about, but as mentioned in the comments on your question List<T> does include a ForEach method.

It would look like this:

emails.ForEach(item =>
{
    item.IsPrimary = item.IsPrimary && item.EmailAddress.Equals(newEmailAddress, StringComparison.InvariantCultureIgnoreCase);
});

edited Mar 02 '18 at 23:01

answered Mar 02 '18 at 22:16

McAden

13,714
5
37
63

while I like the boolean expression, doesn't it set `IsPrimary` incorrectly? (e.g. if `item.IsPrimary` is `false`, it sets it to `true` which is not what the original code did.) Also, it sets every `IsPrimary` whether needed or not. – NetMage Mar 02 '18 at 22:30
@NetMage you're absolutely right, I got in a hurry. It's fixed. Yes it does set each time which _could_ have side-effects if there's extra logic in the setters but as long as that's not the case I wouldn't consider it a performance hit of any real significance. – McAden Mar 02 '18 at 23:04

NetMage · Answer 2 · 2018-03-02T22:38:16.387

0

LINQ is intended for querying and not modification. Having said that, there is a List.ForEach operator, but with no increase in readability most of the time.

Having said that, I personally prefer not having side effect causing code that modifies the collection but I am not opposed to modifying the objects in the collection.

Add an extension method on IEnumerable to encapsulate the foreach loop:

public static void ForEach<T>(this IEnumerable<T> source, Action<T> action) {
    foreach (var s in source)
        action(s);
}

Then you can re-write your code as follows:

emails.Where(item => item.IsPrimary && !item.EmailAddress.Equals(newEmailAddress, StringComparison.InvariantCultureIgnoreCase))
      .ForEach(item => item.IsPrimary = false);

(Thanks to @McAden for the better string comparison I always forget.)

However, since you are creating a race condition anyway, if practical I would suggest reversing your order of operations:

// before adding newEmailAddress
emails[emails.FindIndex(item => item.IsPrimary)].IsPrimary = false; // add error handling if it is possible no `IsPrimary` exists.
// now assign the newEmailAddress and set that item.IsPrimary to true

edited Mar 02 '18 at 22:38

answered Mar 02 '18 at 22:23

NetMage

26,163
3
34
55

1

And what is the benefit of this extension method? How does it make the code more readable that the original? – DavidG Mar 02 '18 at 22:26
@DavidG I don't think that it does, but that is not what the question is about. – NetMage Mar 02 '18 at 22:26
@DavidG at what point did he askt o make it more readable? He just asked how to do this with foreach. – jdmdevdotnet Mar 02 '18 at 22:27
1

@jdmdevdotnet No, but we still have a responsibility to help people *do the right thing*. Sometimes that means saying "don't do that" (and say why) – DavidG Mar 02 '18 at 22:37
@DavidG I tried to say that, but I made it more explicit. – NetMage Mar 02 '18 at 22:38
@NetMage Indeed, you started well but it went rapidly downhill when you wrote the extension method that does nothing but obfuscate ([and already exists](https://msdn.microsoft.com/en-us/library/bwabdf9z(v=vs.110).aspx)!) – DavidG Mar 02 '18 at 22:41
1

@DavidG thats a List extension, _not_ an `IEnumerable` extension like NetMage created. Although I still think the `ForEach` extension for List or IEnumerable are travesties and should not be used that way. – maccettura Mar 02 '18 at 22:45
@DavidG 1) It doesn't exist - or did you miss that I wrote `IEnumerable.ForEach` and applied it to the result of `Where`? 2) Obfuscation is in the eye of the beholder – NetMage Mar 02 '18 at 22:46

Alexei Levenkov · Answer 3 · 2018-03-02T23:56:30.350

-1

You can easily do that, but you should not as no one would expect LINQ code to modify the items in the collection.

emails
    .Where(item =>
       (item.EmailAddress.ToLower() != newEmailAddress.ToLower() && item.IsPrimary)
   .Select(item => { item.IsPrimary = false; return true;})
   .All();

Note that since LINQ queries are actually executed when result is enumerated you need something that will actually enumerate result. I.e. .All() call.

What would happen after you write this code - someone (or you in a week) remove that stupid and pointless .All() call at the end and things will be somewhat ok, but modification no longer happen, person will spend a day sorting it out and then use some words to describe author of the code. Don't go there.

edited Mar 02 '18 at 23:56

answered Mar 02 '18 at 22:29

Alexei Levenkov

98,904
14
127
179

3

I'm pretty sure `.Select.Where` won't compile. – NetMage Mar 02 '18 at 22:31
I'm not going to downvote as I consider the `.Select.Where` to simply be a typo and this _does technically_ do the change in linq. However, if I saw this in my repos I'd totally submit it to thedailywtf and I'd have a good sit-down with the perpetrator. – McAden Mar 02 '18 at 23:11
@McAden (typo fixed, thanks)..I tried to put some minor warning about that it is bad idea in the post, but it somewhat hard to answer this question with sensible recommendation... (Especially with `.ForEach` generally not considered part of LINQ - https://stackoverflow.com/questions/200574/linq-equivalent-of-foreach-for-ienumerablet) – Alexei Levenkov Mar 02 '18 at 23:59

Setting value of object property in List using LINQ

3 Answers3