C#: remove items from an ArrayList with foreach or for loop?

Question

I am quite a noob both with programming and programming with C# (I have learned some basic Java before). I am trying to play around with C# in Unity3D, and I have a question:

Is it better to use for-loop instead of foreach iteration to remove any items in an ArrayList?

Both seems to work for me. Foreach produce slightly less coding but the discussions on the Internet seems to be favoring for-loop. Which is better and why?

for-loop version:

public void RemoveUnitFromList(GameObject Unit) {
    if (SelectedUnitList.Count > 0) {
        for (int i = 0; i < SelectedUnitList.Count; i++) {
            GameObject ArrayListUnit = SelectedUnitList[i] as GameObject;
            if (ArrayListUnit == Unit) {
                SelectedUnitList.RemoveAt(i);
                // some other codes
            }
        }
    }
}

foreach version:

public void RemoveUnitFromList(GameObject Unit) {
    if (SelectedUnitList.Count > 0) {
        foreach (GameObject ArrayListUnit in new ArrayList(SelectedUnitList)) {
            if (ArrayListUnit == Unit) {
                SelectedUnitList.Remove(ArrayListUnit);
                // some other codes
            }
        }
    }
}

Edit: Either of the code snippets above works fine for me. I only want to study the difference between the two. However, changing in new ArrayList(SelectedUnitList) to in SelectedUnitList did not work:

public void RemoveUnitFromList(GameObject Unit) {
    if (SelectedUnitList.Count > 0) {
        foreach (GameObject ArrayListUnit in SelectedUnitList) {
            if (ArrayListUnit == Unit) {
                SelectedUnitList.Remove(ArrayListUnit);
                // some other codes
            }
        }
    }
}

Edit2: The above code could produce the following error:

InvalidOperationException: List has changed.
System.Collections.ArrayList+SimpleEnumerator.MoveNext () (at /Users/builduser/buildslave/mono-runtime-and-classlibs/build/mcs/class/corlib/System.Collections/ArrayList.cs:141)
Mouse3.RemoveUnitFromList (UnityEngine.GameObject Unit) (at Assets/Scripts/Mouse3.cs:216)
Mouse3.Update () (at Assets/Scripts/Mouse3.cs:85)

Let me explains more. The method functions dynamically in game. The SelectedUnitList is an ArrayList used to collect selected unit in a RTS game. The method removes the deselected units from the ArrayList, so Unit could be 0, 1 or more.

Edit3 (last time): Thank you all for all the information provided. I think @Seminda in the comments answered my question.

Answer 1

From MSDN documentation on foreach Loop:

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.

Reference to your code, note following thing:

See details about the ArrayList. It is usually better to use List. Lists not only avoid boxing or unboxing, but they also lead to clearer and less bug-prone code.

UPDATE:

foreach (2nd snippet in your question) works because you initialize new ArrayList.

foreach (GameObject ArrayListUnit in new ArrayList(SelectedUnitList))

You have initialized redundant ArrayList and used it just for iteration. But when you remove from ArrayList within foreach loop it is from actual ArrayList i.e. SelectedUnitList.

Conculsion:

Use for loop to avoid redundancy and removing from the ArrayList.

Answer 2

Both of these are going to be problematic. I would stay way from using foreach, depending on whether a full copy of the array is made or a shallow copy you could get an error such as "Collection was modified; enumeration operation may not execute. removing list item". I think in this case the foreach will work as a deep copy is created. However for large lists this could be inefficient, you can remove items from the array without having to create a copy of the entire array.

I'm guessing there's only one item in the SelectedUnitList that matches Unit? Otherwise the first bit of code using the for loop will fail. Instead you'd want...

public void RemoveUnitFromList(GameObject Unit) {
    if (SelectedUnitList.Count > 0) {
        for (int i = SelectedUnitList.Count -1; i >= 0; i--) {
            GameObject ArrayListUnit = SelectedUnitList[i] as GameObject;
            if (ArrayListUnit == Unit) {
                SelectedUnitList.RemoveAt(i);
                DeactivateProjectorOfObject(ArrayListUnit);
            }
        }
    }
}

This code loops backwards removing items such that the next item iterated to is not affected by the removal of the item in the previous iteration.

Assuming there is only one item matching game unit....

GameObject arrayListUnit = SelectedUnitList.SingleOrDefault(u => u == unit);
DeactivateProjectorOfObject(arrayListUnit);
SelectedUnitList.Remove(unit);

Also I don't know what's going on with Unity developers but you really need to start reading some guidelines on coding standards. No coding standard I've seen has variables starting with a capital.

http://m.friendfeed-media.com/fc2fd89c8be6ace85f7bacedb14181d76ba6c8be

Answer 3

I you plan to perform processing on each object as you decide to remove it, my preferred method is to build a list of items to be removed while doing the final processing, then in a separate loop, remove the items. Also look into using typed lists (System.Collections.Generic.List<>). Untyped lists are just asking for problems at runtime if you accidently get something of the wrong type added to the list. Typed lists get you compile time protection to eliminate that problem.

I have to ask -- if you are passing in a specific game object to be removed, why do you need to loop though anything at all ?? ArrayList.Remove takes an object parameter, and you really should never have the same game object in your selected unit list twice in the first place. If you do you really need to add some checking to the code that builds the SelectedUnitList to prevent adding duplicates. Also.. is it safe to call DeactivateProjectorOfObject twice on the same object if you do happen to have a duplicate ??

In that case your code boils down to:

    public void RemoveUnitFromList(GameObject Unit) {
        DeactivateProjectorOfObject(unit);
        SelectedUnitList.Remove(unit);
    }

Answer 4

It is the same. In most cases I'm using foreach, because I produce less code. I use for only if I want to delete/add some items for current collection.

You can modified a little bit your code if you want to be precise:

public void RemoveUnitFromList(GameObject Unit) 
{
    if (SelectedUnitList.Count == 0) 
        return;

        foreach (GameObject ArrayListUnit in new ArrayList(SelectedUnitList)) 
        {
            if (ArrayListUnit != Unit) 
                continue;

                SelectedUnitList.Remove(ArrayListUnit);
                DeactivateProjectorOfObject(ArrayListUnit);


        }
    }
}