views:

1440

answers:

4

Ok, my actual problem was this: I was implementing an IList<T>. When I got to CopyTo(Array array, int index), this was my solution:

void ICollection.CopyTo(Array array, int index)
{
    // Bounds checking, etc here.
    if (!(array.GetValue(0) is T))
        throw new ArgumentException("Cannot cast to this type of Array.");
    // Handle copying here.
}

This worked in my original code, and still works. But it has a small flaw, which wasn't exposed till I started building tests for it, specifically this one:

public void CopyToObjectArray()
{
    ICollection coll = (ICollection)_list;
    string[] testArray = new string[6];

    coll.CopyTo(testArray, 2);
}

Now, this test should pass. It throws the ArgumentException about not being able to cast. Why? array[0] == null. The is keyword always returns false when checking a variable that is set to null. Now, this is handy for all sorts of reasons, including avoiding null dereferences, etc. What I finally came up with for my type checking was this:

try
{
    T test = (T)array.GetValue(0);
}
catch (InvalidCastException ex)
{
    throw new ArgumentException("Cannot cast to this type of Array.", ex);
}

This isn't exactly elegant, but it works... Is there a better way though?

+4  A: 

There is a method on Type specifically for this, try:

if(!typeof(T).IsAssignableFrom(array.GetElementType()))
justin.m.chase
Using reflection is surely going to be more expensive than just going for the invalid cast.
Jeff Yates
+1 for effort, and a solution that works. Performance wise though, I think I have to side with ffpf, Reflection is probably taking the long route around.
Matthew Scharley
Thinking about it (and having a mental slap), I think this is probably the only way to do this properly.
Jeff Yates
As a single conditional statement anyway, yeah.
Matthew Scharley
It would only be faster to use the try / catch if the cast happened to be correct. If you have an exception thrown and you catch it here then you are going to see a drastic performance hit vs. an extremely minor one with reflection. Don't be afraid of reflection, it is still pretty fast.
justin.m.chase
+1  A: 

List<T> uses this:

try
{
    Array.Copy(this._items, 0, array, index, this.Count);
}
catch (ArrayTypeMismatchException)
{
  //throw exception...
}
Mark Cidade
Ugly. I would rather it just bubble up the exception.
Jonathan Allen
List<T> throws an ArgumentException, which *is* more appropriate
Mark Cidade
Since you're taking the effort though, I'd probably pass the ArrayTypeMismatch as an inner exception though.
Matthew Scharley
List<T> has T already... so it's pretty easy to make sure you have the right type.
justin.m.chase
+3  A: 

The only way to be sure is with reflection, but 90% of the time you can avoid the cost of that by using array is T[]. Most people are going to pass a properly typed array in, so that will do. But, you should always provide the code to do the reflection check as well, just in case. Here's what my general boiler-plate looks like (note: I wrote this here, from memory, so this might not compile, but it should give the basic idea):

class MyCollection : ICollection<T> {
   void ICollection<T>.CopyTo(T[] array, int index) {
       // Bounds checking, etc here.
       CopyToImpl(array, index);
   }
   void ICollection.CopyTo(Array array, int index) {
       // Bounds checking, etc here.
       if (array is T[]) { // quick, avoids reflection, but only works if array is typed as exactly T[]
           CopyToImpl((T[])localArray, index);
       } else {
           Type elementType = array.GetType().GetElementType();
           if (!elementType.IsAssignableFrom(typeof(T)) && !typeof(T).IsAssignableFrom(elementType)) {
               throw new Exception();
           }
           CopyToImpl((object[])array, index);
       }
   }
   private void CopyToImpl(object[] array, int index) {
       // array will always have a valid type by this point, and the bounds will be checked
       // Handle the copying here
   }
}

EDIT: Ok, forgot to point something out. A couple answers naively used what, in this code, reads as element.IsAssignableFrom(typeof(T)) only. You should also allow typeof(T).IsAssignableFrom(elementType), as the BCL does, in case a developer knows that all of the values in this specific ICollection are actually of a type S derived from T, and passes an array of type S[]

Alex Lyman
How do you suggest handling the case where not all elements are of a type S, and a InvalidCastException gets thrown? User beware?
Matthew Scharley
That's the way the BCL does it.
Alex Lyman
The "is" keyword in C# is a reflection call. So you're not saving much by going the long way around the track. Just cut right to the chase and do the IsAssignableFrom.
justin.m.chase
@just in case: Actually in this case "is" is implemented via the "isinst" CIL instruction (ECMA-225 Partition III, Section 4.6). While technically the JIT is allowed to call reflection to implement it, both MS-CLR and Mono have _very_ efficient inline code for it (it also runs every time you unbox)
Alex Lyman
A: 

Here is a little test of try / catch vs. reflection:

object[] obj = new object[] { };
DateTime start = DateTime.Now;

for (int x = 0; x < 1000; x++)
{
    try
    {
        throw new Exception();
    }
    catch (Exception ex) { }
}
DateTime end = DateTime.Now;
Console.WriteLine("Try/Catch: " + (end - start).TotalSeconds.ToString());

start = DateTime.Now;

for (int x = 0; x < 1000; x++)
{
    bool assignable = typeof(int).IsAssignableFrom(obj.GetType().GetElementType());
}
end = DateTime.Now;
Console.WriteLine("IsAssignableFrom: " + (end - start).TotalSeconds.ToString());

The resulting output in Release mode is:

Try/Catch: 1.7501001
IsAssignableFrom: 0

In debug mode:

Try/Catch: 1.8171039
IsAssignableFrom: 0.0010001

Conclusion, just do the reflection check. It's worth it.

justin.m.chase
Of course... you are assuming that it will throw each time!
François
You always make that assumption when calculating performance.
justin.m.chase