CODEWITHSUNDEEP
c#.net.net-3.5c#-3.0
Jon
Jon

Reputation: 40032

C# - Removing Items from Dictionary in while loop

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

Upvotes: 7

Views: 15065

Answers (10)

Sheikh Abdul Wahid
Sheikh Abdul Wahid

Reputation: 2773

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

Upvotes: -1

Andreas Rehm
Andreas Rehm

Reputation: 2232

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.

Upvotes: 12

Star
Star

Reputation: 49

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)

Upvotes: 0

Meiscooldude
Meiscooldude

Reputation: 3739

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

Upvotes: 0

JSprang
JSprang

Reputation: 12909

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

Upvotes: 0

Justin Niessner
Justin Niessner

Reputation: 245429

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

Upvotes: 0

Donut
Donut

Reputation: 112825

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

Upvotes: 0

SwDevMan81
SwDevMan81

Reputation: 49978

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

Upvotes: 1

Reed Copsey
Reed Copsey

Reputation: 564413

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

Upvotes: 1

JaredPar
JaredPar

Reputation: 754745

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

Upvotes: 11

Related Questions