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();

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() :)

Answer 2

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();

Answer 3

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

Answer 4

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

Answer 5

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

Answer 6

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(); 

Answer 7

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

Answer 8

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)

Answer 9

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.