17

I'm writing a class that implements ICollection<T> and ICollection interfaces.

MSDN says these are a bit different. ICollection<T>.CopyTo takes a T[] argument, whereas ICollection.CopyTo takes a System.Array argument. There is also a difference between exceptions thrown.

Here is my implementation of the generic method (I believe it's fully functional):

void ICollection<PlcParameter>.CopyTo(PlcParameter[] array, int arrayIndex)
{
    if (array == null)
        throw new ArgumentNullException("array");
    if (arrayIndex < 0)
        throw new ArgumentOutOfRangeException("arrayIndex");
    if (array.Length - arrayIndex < Count)
        throw new ArgumentException("Not enough elements after arrayIndex in the destination array.");

    for (int i = 0; i < Count; ++i)
        array[i + arrayIndex] = this[i];
}

However, the non-generic version of the method is confusing me a bit. First, how do I check for the following exception condition?

The type of the source ICollection cannot be cast automatically to the type of the destination array.

Second, is there a way to leverage the existing generic implementation to reduce code duplication?

Here is my work-in-progress implementation:

void ICollection.CopyTo(Array array, int index)
{
    if (array == null)
        throw new ArgumentNullException("array");
    if (index < 0)
        throw new ArgumentOutOfRangeException("arrayIndex");
    if (array.Rank > 1)
        throw new ArgumentException("array is multidimensional.");
    if (array.Length - index < Count)
        throw new ArgumentException("Not enough elements after index in the destination array.");

    for (int i = 0; i < Count; ++i)
        array.SetValue(this[i], i + index);
}
relatively_random
  • 4,505
  • 1
  • 26
  • 48
  • 1
    Well `SetValue` will definitely throw a cast exception for you. – juharr Feb 15 '16 at 15:00
  • 2
    Can you implement your class by deriving from [`CollectionBase`](https://msdn.microsoft.com/en-us/library/system.collections.collectionbase_methods%28v=vs.110%29.aspx) to save work? It implements `CopyTo()` for you. – Matthew Watson Feb 15 '16 at 15:02
  • have a look to this question - http://stackoverflow.com/questions/5843958/how-to-make-a-copy-of-a-reference-type – harmoniemand Feb 15 '16 at 15:02
  • @juharr catching the `CastException` to throw the `ArgumentException` is needless expense though. – Jon Hanna Feb 15 '16 at 15:06
  • On second thoughts, `CollectionBase` is ancient and predates generics, so perhaps that's not a good idea... – Matthew Watson Feb 15 '16 at 15:10
  • 1
    There is Collection instead that is generic and basically wraps a List with extension points. Its specifically for writing custom generic collections. – Mant101 Feb 15 '16 at 15:14
  • @MatthewWatson So is ICollection but not that I can escape it here. :) CollectionBase wouldn't suit me because the collection needs to be read-only. But I wasn't aware of ReadOnlyCollectionBase until now so I'm looking into it. As you can see, not an expert here. – relatively_random Feb 15 '16 at 15:17
  • @HansPassant Don't I need to be able to cast the class I'm writing into an Array for that? And wouldn't that step potentially add bugs as well? – relatively_random Feb 15 '16 at 15:18
  • 1
    @HansPassant that's assuming the underlying implementation is an array, which it might not be. – Jon Hanna Feb 15 '16 at 15:29

2 Answers2

6

You've already done most of the work in implementing ICollection<T>.CopyTo.

There are four possibilities for ICollection.CopyTo:

  1. It will work the same as ICollection<T>.
  2. It will fail for a reason ICollection<T> would have failed.
  3. It will fail because of rank-mismatch.
  4. It will fail because of type mismatch.

We can handle the first two by calling into ICollection<T>.CopyTo.

In each of these cases array as PlcParameter[] will give us a reference to the array, strongly typed.

In each of the latter cases, it won't.

We do want to catch array == null separately though:

void ICollection.CopyTo(Array array, int index)
{
  if (array == null)
    throw new ArgumentNullException("array");
  PlcParameter[] ppArray = array as PlcParameter[];
  if (ppArray == null)
    throw new ArgumentException();
  ((ICollection<PlcParameter>)this).CopyTo(ppArray, index);
}

If you really wanted you could test array.Rank == 1 in the case of ppArray being null, and change the error message accordingly.

(BTW, why are you explicitly implementing ICollection<PlcParameter>.CopyTo? it's likely enough to be useful to be work implementing explicitly, so people don't have to cast to all it.)

Jon Hanna
  • 110,372
  • 10
  • 146
  • 251
  • Thank you very much, this seems like the perfect solution. I didn't think of casting to `T[]` for some reason. I'll also take your advice about not implementing explicitly. – relatively_random Feb 15 '16 at 15:33
  • If you do that then you can change the rather ugly `((ICollection)this).CopyTo(ppArray, index)` to just `CopyTo(ppArray, index)`. – Jon Hanna Feb 15 '16 at 15:37
3

In the non-generic CopyTo the following cases can happen:

  • Types actually match: call the generic version
  • The array is object[]: Handle this one as well, especially if T is a reference type since in this case T[] can be cast to object[]
  • If your collection is a dictionary, you can handle the KeyValuePair[] and DictionaryEntry[] arrays as well

So in a general case you should use the following implementation:

void ICollection.CopyTo(Array array, int index)
{
    if (array != null && array.Rank != 1)
        throw new ArgumentException("Only single dimensional arrays are supported for the requested action.", "array");

    // 1. call the generic version
    T[] typedArray = array as T[];
    if (typedArray != null)
    {
        CopyTo(typedArray, index);
        return;
    }

    // 2. object[]
    object[] objectArray = array as object[];
    if (objectArray != null)
    {
        for (int i = 0; i < size; i++)
        {
            objectArray[index++] = GetElementAt(i);
        }
    }

    throw new ArgumentException("Target array type is not compatible with the type of items in the collection.");
}
György Kőszeg
  • 17,093
  • 6
  • 37
  • 65