3

So I have tried to create some basic extension methods for List. Essentially I have a UniqueAdd and UniqueAddRange. It will check for the existence of a value before adding and if its already in the List it will not add it. Here is the code:

public static class ListExtensions
{
    /// <summary>
    /// Adds only the values in the 'values' collection that do not already exist in the list. Uses list.Contains() to determine existence of
    /// previous values.
    /// </summary>
    /// <param name="list"></param>
    /// <param name="values"></param>
    public static void UniqueAddRange<T>(this List<T> list, IEnumerable<T> values)
    {
        foreach (T value in values)
        {
            list.UniqueAdd(value);
        }
    }

    /// <summary>
    /// Adds the value to the list only if it does not already exist in the list. Uses list.Contains() to determine existence of previos values.
    /// </summary>
    /// <typeparam name="T"></typeparam>
    /// <param name="list"></param>
    /// <param name="value"></param>
    public static void UniqueAdd<T>(this List<T> list, T value)
    {
        if (!list.Contains(value))
        {
            list.Add(value);
        }
    }
}

And I get the following error when building:

CA0001 : Rule=Microsoft.Maintainability#CA1506, Target=Some.Namespace.ListExtensions : Collection was modified; enumeration operation may not execute.

Here is the link to the error but I'm not sure how to fix my extension methods given this information. It says to

Try to redesign the type or method to reduce the number of types to which it is coupled.

Does anyone know why I'm getting this error and how to fix my extension methods so they do not violate this rule?

Thanks!

PS: Before anyone mentions it, I have considered using a HashSet but HashSet does not exist in the compact framework.

Ian Dallas
  • 12,451
  • 19
  • 58
  • 82

2 Answers2

4

I think your code triggered a bug in FxCop, "Collection was modified" is a classic oops. It then decided that its bug was your problem, catch(Exception) style.

Look for an update. The one I use doesn't complain about your code (VS2010 version).

Hans Passant
  • 922,412
  • 146
  • 1,693
  • 2,536
1

It's telling you that you are changing the list as you are enumerating it. This is clear from your code (you're adding to the list at the same time as enumerating).

How about:

public static void UniqueAddRange<T>(this List<T> list, IEnumerable<T> values)
{
    list.AddRange(values.Except(list));
}

Or, if the interface is right for your needs, use a Hashset. It does what you want out of the box.

spender
  • 117,338
  • 33
  • 229
  • 351
  • Thanks, I'll give this a try. I forgot that Contains will inadvertently enumerate the list which is why I am getting this issue. As for the HashSet, I mentioned in my post that it doesn't exist in the compact framework. – Ian Dallas Sep 29 '11 at 23:42
  • No, you're seeing this issue because the foreach loop in `UniqueAddRange` is iterating an enumeration that is being changed under its feet. Spotted the Hashset thing, hence the strikethrough ;) The LINQ way *should* be quicker too because it doesn't have to chack the entire list for every insertion. – spender Sep 29 '11 at 23:43
  • I don't think that's correct: `values` and `list` are two completely seperate lists and I only iterate over `values`, I never modify it. I only modify `list`. – Ian Dallas Sep 29 '11 at 23:53
  • Ah, yes. My bad. I should go to bed now, as I'm useless! I'd venture that the code I provided is probably more efficient. Looks like @Hans might be right. – spender Sep 29 '11 at 23:54
  • I don't doubt that in the full .NET framework it is but surprisingly the compact framework is quite slow with LINQ. – Ian Dallas Sep 30 '11 at 00:13
  • There's a rather fundamental problem with this answer: it assumes that the code is actually executed. It is not, this error is generated during *code analysis*. FxCop, it only looks at the IL, it doesn't execute it. – Hans Passant Oct 02 '11 at 21:56