How to remove elements from an array

Hi I'm working on some legacy code that goes something along the lines of

for(int i = results.Count-1; i >= 0; i--)
{
  if(someCondition)
  {
     results.Remove(results[i]);
  }
}

To me it seems like bad practice to be removing the elements while still iterat开发者_运维问答ing through the loop because you'll be modifying the indexes.

Is this a correct assumption?

Is there a better way of doing this? I would like to use LINQ but I'm in 2.0 Framework

The removal is actually ok since you are going downwards to zero, only the indexes that you already passed will be modified. This code actually would break for another reason: It starts with results.Count, but should start at results.Count -1 since array indexes start at 0.

for(int i = results.Count-1; i >= 0; i--)
{
  if(someCondition)
  {
     results.RemoveAt(i);
  }
}

Edit:

As was pointed out - you actually must be dealing with a List of some sort in your pseudo-code. In this case they are conceptually the same (since Lists use an Array internally) but if you use an array you have a Length property (instead of a Count property) and you can not add or remove items.

Using a list the solution above is certainly concise but might not be easy to understand for someone that has to maintain the code (i.e. especially iterating through the list backwards) - an alternative solution could be to first identify the items to remove, then in a second pass removing those items.

Just substitute MyType with the actual type you are dealing with:

List<MyType> removeItems = new List<MyType>();

foreach(MyType item in results)
{
   if(someCondition)
   {
        removeItems.Add(item);
   }
}

foreach (MyType item in removeItems)
    results.Remove(item);

It doesn't seem like the Remove should work at all. The IList implementation should fail if we're dealing with a fixed-size array, see here.

That being said, if you're dealing with a resizable list (e.g. List<T>), why call Remove instead of RemoveAt? Since you're already navigating the indices in reverse, you don't need to "re-find" the item.

May I suggest a somewhat more functional alternative to your current code:

Instead of modifying the existing array one item at a time, you could derive a new one from it and then replace the whole array as an "atomic" operation once you're done:

The easy way (no LINQ, but very similar):

Predicate<T> filter = delegate(T item) { return !someCondition; };

results = Array.FindAll(results, filter);
// with LINQ, you'd have written: results = results.Where(filter);

where T is the type of the items in your results array.

A somewhat more explicit alternative:

var newResults = new List<T>();
foreach (T item in results)
{
    if (!someCondition)
    {
        newResults.Add(item);
    }
}
results = newResults.ToArray();

Usually you wouldn't remove elements as such, you would create a new array from the old without the unwanted elements.

If you do go the route of removing elements from an array/list your loop should count down rather than up. (as yours does)

a couple of options:

List<int> indexesToRemove = new List<int>();
for(int i = results.Count; i >= 0; i--)
{
  if(someCondition)
  {
     //results.Remove(results[i]);
       indexesToRemove.Add(i);
  }
}

foreach(int i in indexesToRemove) {
results.Remove(results[i]);
}

or alternatively, you could make a copy of the existing list, and instead remove from the original list.

//temp is a copy of results
for(int i = temp.Count-1; i >= 0; i--)
{
  if(someCondition)
  {
     results.Remove(results[i]);
  }
}