1

I know there has to be a better way to do this... maybe with LINQ? If I remove a value, it invalidates the iterator which is why I have this inside of an infinite loop which starts the process all over again. I'm looking for a way to do this that is easier to read, maintain and ultimately much faster. Here's what I got:

Dictionary<string, string> Channels = //...;
while (true)
{
    var bFound = false;
    foreach(var c in Channels)
    {
        if(c.Value == version)
        {
            Channels.Remove(c.Key);
            bFound = true;
            break;
        }
    }
    if (!bFound) { break; }
}

Thanks for any help in optimizing this routine.

13
  • what does your dictionary look like? Commented May 7, 2018 at 21:06
  • @PatrickArtner -- updated question with more context Commented May 7, 2018 at 21:08
  • Why is this in an infinite loop? "invalidates the iterator" does not makes sense. Commented May 7, 2018 at 21:08
  • 1
    @charliefox2 OP is checking the value, it is spelled out multiple times in the post. Look at the code, look at the title Commented May 7, 2018 at 21:19
  • 1
    All the answers below are omitting the important remark in the OP post I'm looking for a way to do this that is easier to read, maintain and ultimately much faster. Just use a for loop :) Commented May 7, 2018 at 21:20

3 Answers 3

7

Based on your example it appears you are removing based on comparison with version value.

Channels = Channels.Where(x=> x.Value != version)
              .ToDictionary(c => c.Key, c => c.Value);
Sign up to request clarification or add additional context in comments.

Comments

3

I'm looking for a way to do this that is easier to read, maintain and ultimately much faster

Just use the code below:

var keys = Channels.Keys.ToArray();
foreach(var key in keys)
{
    if(Channels[key] == version)
    {
        Channels.Remove(key);
    }
}

No LINQ needed for simplicity. We traverse the dictionary once for performance.

8 Comments

there is a small error. It needs to be if(Channels[key]== version)
Did you try it? The documentation for Keys indicates that changes to the underlying collection will be reflected, which may indicate that the enumerator will be invalidated.
@Andy I fixed my answer :).
> The returned Dictionary<TKey, TValue>.KeyCollection is not a static copy; instead, the Dictionary<TKey, TValue>.KeyCollection refers back to the keys in the original Dictionary<TKey, TValue>. Therefore, changes to the Dictionary<TKey, TValue> continue to be reflected in the Dictionary<TKey, TValue>.KeyCollection. msdn.microsoft.com/en-us/library/yt2fy5zk(v=vs.110).aspx
Good fix, I tried the old code and got System.InvalidOperationException : Collection was modified; enumeration operation may not execute.
|
2

If you capture the matches before removing items from the dictionary, your enumerator will not be invalidated.

List<KeyValuePair<string, string>> matches = Channels
  .Where(kvp => kvp.Value == version)
  .ToList();

foreach(KeyValuePair<string, string> match in matches)
{
  Channels.Remove(match.Key);
}

2 Comments

Wouldnt it be better to avoid the call to ToList(); ?
Suppose the ToList call is removed, then matches is now a deferred query with a source of Channels. Once Channels is modified the deferred query will be unable to produce its next element.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.