Share via

Removing elements during iteration through a Dictionary

  • Article

Question

Thursday, April 28, 2005 7:55 PM

I'm trying to iterate through a Dictionary and remove some of the elements according to some criterion. I get an InvalidOperationException that says: "Collection was modified; enumeration operation may not execute."


IDictionary<IPAddress, DateTime> peers = new Dictionary<IPAddress, DateTime>(); foreach (KeyValuePair<IPAddress, DateTime> peer in peers) {     if (peer.Value < DateTime.Now) {        peers.Remove(peer.Key);     } } 
 

The condition given is not the actual condition (as it would remove all elements from the collection).

All replies (9)

Thursday, April 28, 2005 8:53 PM âś…Answered

If you need to modify the collection while iterating through it, you cannot use the foreach keyword. foreach uses an enumerator that is invalidated when the underlying collection changes. (An enumerator typically stores something like an integer index into a collection. So it's not robust in the face of collection changes.) The simple solution... Use a regular for loop and loop backwards.

for(int j=peers.Count-1; j>=0; j--) {
  KeyValuePair<IPAddress, DateTime> peer = peers[j];
  if(peer.Value < DateTime.Now) {
    peers.Remove(peer.Key);
  }
}

Note that access to the collection is not thread-safe. So you need to provide appropriate locking around access to peers. (i.e. You could throw an IndexOutOfRangeException if you want peers[peers.Count-1] and another thread removes the item before you can retrieve it.)

Sunday, July 17, 2005 10:43 AM

Hi,

That's a nice solution but I have a similar problem to the originator and it is not solved by the code given above.

I have a Dictionary structure wants a string in the array element - eg m_mapCatalogs["bla"].
That is to say, I can't type m_mapCatalogs[4] and compile.

My problem summarily is this: The only access I can see into my Dictionary is via an Enumerator. If I can't delete while iterating, then I can't selectively delete.
There must be a sensible way around this that I can't see yet?



private Dictionary<string, Catalog> m_mapCatalogs = new Dictionary<string, Catalog>();

 


//TODO InvalidOperationException because enumeration subject is modified during enumeration
//foreach (Catalog cat in mapCatalogs) {
//    if (cat.CheckedState == CheckState.Checked) {
//        m_mapCatalogs.Remove(cat);
//    }
//}
 

In C++, I did this (probably stupid) thing:



void CatalogManager::RemoveSelected()
{
    CatalogMap::iterator cat = m_mapCatalogs.begin();
    while ( cat != m_mapCatalogs.end() )
    {
        if ( (*cat).second.IsSelected() ) {
             Remove( (*cat).second.getName() );
            cat = m_mapCatalogs.begin();    // we need this to guarantee it will work
        }                                    // this is stupid, fix it
        else {
            cat++;
        }
    }

 

Sunday, July 17, 2005 1:36 PM

You're not missing anything, but James apparently didn't notice that you were asking about Dictionary.  His reply is correct for ArrayList/List which have a positional (Int32) indexer, but does not apply to Hashtable/Dictionary and similar collections which only have a key indexer.

So you're out of luck, basically.  What you could do is copy the Keys collection to a temporary list, and then iterate over that list, using the current key as an indexer into the dictionary.

I have to admit that I don't understand the second code sample in your lats post, though. What's catalogList, and how would its enumeration affect m_mapCatalogs?

Monday, July 18, 2005 3:20 AM

dont know why you want to use a dictionary. . .  I bet it isnt necessary. . .

What you need is a CatalogManager List Generic that factories out catalogs. . .
A Catalog raises an event when checked is changed. . .

The event is wired to an methog on the Catalog manager that adds/removes the catalog from an internal catalog manager , m_checked. . .

Implement IDisposable on catalog. . . wire an event in the dispose method that removes it from its manager. . .

enumerate m_checked to remove the catalog from the List of Managed Catalogs


class CatalogManager: System.Collections.Generic.List<Catalog>
{
    CatalogManager m_checked = new CatalogManager();

    public Catalog CreateCatalog()
    {
        Catalog cat = new Catalog();
        cat.CheckedChanged += new EventHandler(HandleCatalogCheckedChanged);
        cat.Disposed += new EventHandler(OnCatalogDisposed);
        this.Add(cat);
        return cat;
    }

    void HandleCatalogCheckedChanged(object sender, EventArgs e)
    {
        Catalog cat = sender as Catalog;
        if (cat == null) return;
        if (cat.Checked)
            m_checked.Add(cat);
        else
            m_checked.Remove(cat);
    }

    public void RemoveCheckedCatalogs()
    {
        foreach (Catalog cat in m_checked)
            this.Remove(cat);
        m_checked.Clear();
    }

    private void OnCatalogDisposed(object sender,EventArgs e)
    {
        Catalog cat = sender as Catalog;
        if (cat == null) return;
        m_checked.Remove(cat);
        this.Remove(cat);
    }
}

class Catalog:IDisposable, ICatalog
{

    bool m_checked = false;
    bool m_disposed = false;

    internal Catalog(){}

    public event EventHandler CheckedChanged;
    public event EventHandler Disposed;

    public bool Checked
    {
        get
        {
            return m_checked;
        }
        set
        {
            if (value != m_checked) return;
            m_checked = value;
            if (CheckedChanged != null)
                CheckedChanged(this, EventArgs.Empty);
        }
    }

    private void Dispose(bool disposing)
    {
        if(!this.m_disposed)
        {
            if(disposing)
                if (Disposed !=null)
                    Disposed(this, EventArgs.Empty);
        }
        m_disposed = true;        
    }

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }
}

public interface ICatalog
{
    bool Checked { get; set;}
    event EventHandler CheckedChanged;
}
 

Make sense???

Monday, July 18, 2005 3:40 AM

Oh yeah. . .

Try to forget everything you knew about VB6 Collections. They were pieces of cr*p and the root of everything that was bad about VB. They made things much more difficult than they had to be and trying to do things that way will just make things harder in .Net.

Remember object-oriented means orient to the object not to the process! 

Someone should take the VB team out and shoot them for unleashing that dog on an unsuspecting public!!!

Tuesday, July 19, 2005 7:45 AM

 Chris Nahr wrote:
I have to admit that I don't understand the second code sample in your lats post, though. What's catalogList, and how would its enumeration affect m_mapCatalogs?

Hi Chris,

Thanks, yes you're not wrong about being confused. I must have been refactoring the code and forgot to set it back. catalogList should have been m_mapCatalogs.
I've corrected the original.

Tuesday, July 19, 2005 7:50 AM

Thanks Blair.
I think you're right, I don't need a HashMap. I like your idea. Inheriting from a GenericList probably is a much better idea!

I'm not quite sure if the dispose() is required, but thanks for the effort. Certainly made it a lot easier for me to grasp than if you'd just written about it using just English grammar.

Tuesday, July 19, 2005 9:08 AM

you need the dispose implementation to fire the Disposed event that that assures removal of the reference from the Managers m_Checked list else you will get a memory leak because the GC won't clean it up if the reference remains there. . .  

Then again, if the only way in which you clear a Catalog is by calling RemoveCheckedCatalogs, it might not be necessary, but just to be on the safe side. . . .

and now that I think about it. . . .

RemoveCheckedCatalogs should be defined simply as- 

public void RemoveCheckedCatalogs()
{
        foreach (Catalog cat in m_checked)
                cat.Dispose();
}

At anyrate, I think this approach will open up the creativity well for you. 

Tuesday, July 19, 2005 11:59 PM

Hi Blair,

In fact, further to your idea, given the specifics of my scenario, I think I could probably inherit the CatalogManager from the CheckedListBox... ? That is, I could be even more specific than a Generic List.

Basically, I suppose, my CatalogManager is a wrapper around the CheckedListBox but with extra features where it assumes that the list items are filenames/paths. So it opens them, and searches them for lines containing certain text.

I don't know if it's a logical stretch to inherit from the CheckedListBox, but I sort of like it.

The problem if I don't is, I have to maintain a CheckedListBox internally anyway... and each time an item is added to the CheckedListBox I have to synchronise them and make sure their in tune.
If I inherit from the CheckedListBox, then my CatalogManager *is* the list and I avoid the double-code synchronisation altogether.

I might be on a dodgy track, but you've got me thinking in the right direction. Thanks very much for that. It's a good feeling to get out of the brain-hole I was obviously stuck in.