C# - Removing Items from Dictionary in while loop

Question

I have this and all seems to work fine but not sure why and if its valid.

        Dictionary<string, List<string>> test = new Dictionary<string, List<string>>();

        while (test.Count > 0)
        {
            var obj = test.Last();
            MyMethod(obj);
            test.Remove(obj.Key);
        }

Update: Thanks for the answers, I have updated my code to explain why I don't do Dictionary.Clear();

Cortese

Answer 1

There is nothing wrong with mutating a collection type in a while loop in this manner. Where you get into trouble is when you mutate a collection during a foreach block. Or more generally use a IEnumerator<T> after the underlying collection is mutated.

Although in this sample it would be a lot simpler to just call test.Clear() :)

Dichroite

Answer 2

I don't understand why you are trying to process all Dictonary entries in reverse order - but your code is OK.

It might be a bit faster to get a list of all Keys and process the entries by key instead of counting again and again...

E.G.:

var keys = test.Keys.OrderByDescending(o => o).ToList();

foreach (var key in keys)
{
    var obj = test[key];
    MyMethod(obj);
    test.Remove(key);
}

Dictonarys are fast when they are accessed by their key value. Last() is slower and counting is not necessary - you can get a list of all (unique) keys.

Radack

Answer 3

That works, fine, since you're not iterating over the dictionary while removing items. Each time you check test.Count, it's like it's checking it from scratch.

That being said, the above code could be written much simpler and more effectively:

test.Clear();

Darkness

Answer 4

It works because Count will be updated every time you remove an object. So say count is 3, test.Remove will decriment the count to 2, and so on, until the count is 0, then you will break out of the loop

Woodbury

Answer 5

Yes, this should be valid, but why not just call Dictionary.Clear()?

Harl

Answer 6

All you're doing is taking the last item in the collection and removing it until there are no more items left in the Dictionary.

Nothing out of the ordinary and there's no reason it shouldn't work (as long as emptying the collection is what you want to do).

Wichern

Answer 7

So, you're just trying to clear the Dictionary, correct? Couldn't you just do the following?

Dictionary<string, List<string>> test = new Dictionary<string, List<string>>();
        test.Clear(); 

Nara

Answer 8

This seems like it will work, but it looks extremely expensive. This would be a problem if you were iterating over it with a foreach loop (you can't edit collections while your iterating).

Dictionary.Clear() should do the trick (but you probably already knew that).

Interoffice

Answer 9

Despite your update, you can probably still use clear...

foreach(var item in test) {
  MyMethod(item);
}
test.Clear()

Your call to .Last() is going to be extremely inefficient on a large dictionary, and won't guarantee any particular ordering of the processing regardless (the Dictionary is an unordered collection)

Comp

Answer 10

I used this code to remove items conditionally.

var dict = new Dictionary<String, float>
var keys = new String[dict.Count];
dict.Keys.CopyTo(keys, 0);

foreach (var key in keys) {
var v = dict[key];
if (condition) {
    dict.Remove(key);
}
          

Amour

whoever put -1 please mention the reason so I can improve the answer. – Amour