9
\$\begingroup\$

Sometimes you want to add/remove items from a collection inside of a foreach loop. Since this isn't possible, a common pattern is to add the items to a separate collection, then add/remove those items to the original collection after the foreach is finished.

I'm wondering if this can be automated.

Here's one attempt. I haven't tested this too extensively, but it does pass my tests. Also, since I have very little experience with threads, I have not tried to make it thread safe in any way.

Does this approach work? Are there any potential problems? Can it be improved?

#region ForwardingCollection<T>

// A utility class for collection wrappers
public class ForwardingCollection<T> : ICollection<T>
{
    public ICollection<T> Items { get; private set; }

    public ForwardingCollection(ICollection<T> coll)
    {
        Items = coll;
    }

    public virtual void Add(T item)
    {
        Items.Add(item);
    }

    public virtual void Clear()
    {
        Items.Clear();
    }

    public virtual bool Contains(T item)
    {
        return Items.Contains(item);
    }

    public virtual void CopyTo(T[] array, int arrayIndex)
    {
        Items.CopyTo(array, arrayIndex);
    }

    public virtual int Count
    {
        get { return Items.Count; }
    }

    public virtual bool IsReadOnly
    {
        get { return Items.IsReadOnly; }
    }

    public virtual bool Remove(T item)
    {
        return Items.Remove(item);
    }

    public virtual IEnumerator<T> GetEnumerator()
    {
        return Items.GetEnumerator();
    }

    System.Collections.IEnumerator System.Collections.IEnumerable.GetEnumerator()
    {
        return GetEnumerator();
    }
}

#endregion

#region LoopingCollection<T>

// Probably not a good name
public class LoopingCollection<T> : ForwardingCollection<T>
{
    #region FlushingEnumerator<T>

    private class FlushingEnumerator<T> : IEnumerator<T>
    {
        private LoopingCollection<T> coll;
        private IEnumerator<T> enumerator;

        public FlushingEnumerator(LoopingCollection<T> coll, IEnumerator<T> enumerator)
        {
            this.coll = coll;
            this.enumerator = enumerator;

            coll.numLoops++;
        }

        public T Current
        {
            get { return enumerator.Current; }
        }

        public void Dispose()
        {
            enumerator.Dispose();
        }

        object System.Collections.IEnumerator.Current
        {
            get { return Current; }
        }

        public bool MoveNext()
        {
            bool hasNext = enumerator.MoveNext();
            if (!hasNext)
            {
                coll.numLoops--;
                if (coll.numLoops == 0)
                {
                    coll.flush();
                }
            }
            return hasNext;
        }

        public void Reset()
        {
            enumerator.Reset();
        }
    }

    #endregion

    // Keep track of how many loops are enumerating this collection.
    // This is needed for nested loops.
    private int numLoops = 0;

    private List<T> addedItems = new List<T>();
    private List<T> removedItems = new List<T>();

    public LoopingCollection(ICollection<T> coll)
        : base(coll)
    {

    }

    public override void Add(T item)
    {
        if (numLoops == 0)
        {
            Items.Add(item);
        }
        else
        {
            addedItems.Add(item);

            // If there's a pending remove, cancel it
            removedItems.Remove(item);
        }
    }

    public override bool Remove(T item)
    {
        if (numLoops == 0)
        {
            return Items.Remove(item);
        }

        // If there's a pending add, cancel it
        if (addedItems.Remove(item))
        {
            return true;
        }

        if (Items.Contains(item))
        {
            removedItems.Add(item);
            return true;
        }

        return false;
    }

    public override IEnumerator<T> GetEnumerator()
    {
        return new FlushingEnumerator<T>(this, Items.GetEnumerator());
    }

    private void flush()
    {
        foreach (var item in addedItems)
            Items.Add(item);
        addedItems.Clear();

        foreach (var item in removedItems)
            Items.Remove(item);
        removedItems.Clear();
    }
}

#endregion
\$\endgroup\$
8
\$\begingroup\$

Your logic has a serious flaw: you expect that the consumer of the collection will iterate it till the end. In reality there are break or return keywords, and exceptions that will not follow your expected workflow, and thus you will be stuck with positive numLoops. Note that even moving coll.numLoops-- to Dispose method won't help, as you cannot enforce the user of your collection to call it in case they iterate manually, without foreach.

I think the whole idea of working around this limitation (which was placed intentionally by the .NET team) is not very good. It would be good if you provide examples where you think you will benefit from a collection like this in separate code review posts, and community may suggest a better approach.

\$\endgroup\$
  • \$\begingroup\$ Yeah, there's a reason why they patched 4.5 to make .ForEach throw when the collection is modified. I'm curious to see what kind of situations require this. \$\endgroup\$ – moarboilerplate Mar 9 '15 at 15:14
  • \$\begingroup\$ According to this: msdn.microsoft.com/en-us/library/aa664754%28v=vs.71%29.aspx the enumerator is disposed after the foreach, so moving the code from MoveNext to the enumerator's Dispose method will work for the cases you mentioned. \$\endgroup\$ – Gage64 Mar 10 '15 at 21:46
  • \$\begingroup\$ But it's also true that the consumer can use the enumerator manually without disposing it... \$\endgroup\$ – Gage64 Mar 10 '15 at 21:49
3
\$\begingroup\$

This is more of a style nitpick, but I think it helps with the readability of the code

your code:

private void flush()
{
    foreach (var item in addedItems)
        Items.Add(item);
    addedItems.Clear();

    foreach (var item in removedItems)
        Items.Remove(item);
    removedItems.Clear();
}

How I would write it:

private void flush()
{
    foreach (var item in addedItems)
    {
        Items.Add(item);
    }
    addedItems.Clear();

    foreach (var item in removedItems)
    {
        Items.Remove(item);
    }
    removedItems.Clear();
}

I think that it is a good idea to always use Curly Braces, especially because there is a lot of stuff going on in this little method, you are changing 3 different collections in 4 different operations, and we don't want to mix them up.

\$\endgroup\$
  • 1
    \$\begingroup\$ An alternative to curly braces is to simply add a blank line between logical blocks of code. \$\endgroup\$ – craftworkgames Mar 6 '15 at 23:37
  • 1
    \$\begingroup\$ I agree about the curly braces, I somehow missed it. \$\endgroup\$ – Gage64 Mar 7 '15 at 7:53
  • 3
    \$\begingroup\$ No @craftworkgames. There is no alternative to curly braces. \$\endgroup\$ – RubberDuck Mar 9 '15 at 14:56
  • 2
    \$\begingroup\$ @craftworkgames blank lines are not Curly Braces, Curly Braces defines a block of code, a blank line doesn't define anything. \$\endgroup\$ – Malachi Mar 9 '15 at 14:56
  • 1
    \$\begingroup\$ I thought we were talking about nitpicking readability. Curly braces in the above code don't make the code more syntactically or logically correct. They simply serve to make the code more readable. Blank lines are a perfectly valid alternative to make the code more readable. This is a personal preference, it's not a requirement. \$\endgroup\$ – craftworkgames Mar 9 '15 at 20:26
1
\$\begingroup\$

You can't modify a collection in a foreach loop for good reason. However, it is okay to modify a collection using a for loop if you're careful. This is backed up by the MSDN documentation.

The foreach statement is used to iterate through the collection to get the information that you want, but can not be used to add or remove items from the source collection to avoid unpredictable side effects. If you need to add or remove items from the source collection, use a for loop.

For example, in game development you'll often find yourself writing code that loops through a collection of items calling update on each one. During the update the item might be flagged for removal, or a new item might even be added.

        for (int i = 0; i < Animations.Count; i++)
        {
            var animation = Animations[i];
            animation.Update(deltaTime);

            if(animation.IsComplete)
                Animations.Remove(animation);
        }

An alternative to removing them inside the loop is to remove them after iterating through the collection as a separate operation.

        Animations.RemoveAll(i => i.IsComplete);

If performance is not a concern, yet another option is to make a copy of the collection before using it in the foreach loop like this:

        foreach (var animation in Animations.ToArray())
        {
            animation.Update(deltaTime);

            if(animation.IsComplete)
                Animations.Remove(animation);
        }

All of these work fine and avoid the need to write a complicated custom class.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.