tags:

views:

125

answers:

5

In a previous question today these two different approaches was given to a question as answers.

We have an object that might or might not implement IDisposable. If it does, we want to dispose it, if not we do nothing. The two different approaches are these:

1)

if(toDispose is IDisposable)
  (toDispose as IDisposable).Dispose();

2)

IDisposable disposable = toDispose as IDisposable;
if( disposable != null )
  disposable.Dispose();

Mainly, from the comments it sounds like the consensus was that 2) was the best approach.

But looking at the differences, we come down to this:

1) Perform a cast twice on toDispose.

2) Only perform the cast once, but create a new intermediate object.

I would guess that 2 will be marginally slower because it has to allocate a new local variable, so why is this regarded as the best solution in this case? It is solely because of readability issues?

+9  A: 

Not really answering your question but I would recommend you this construct:

using (toDispose as IDisposable)
{
    ...
}

and let the compiler worry about the ifs, finally and Dispose calls.

Darin Dimitrov
+1. Just saying using (toDispose) is enough, as it will dispose only if its IDisposable.
eglasius
@eglasius, but it might not compile depending on the type of `toDispose`. For example: `object toDispose = FetchSomeDisposableResource();`.
Darin Dimitrov
@Darin that's right, I actually double checked it. My memory failed, I recalled doing that a very long time ago, but I guess I must have used an as IDisposable as well ... and was really just about the using statement ignoring null.
eglasius
+9  A: 

My rules of thumb around casting:

  • If it's an error/bug for the value not to be of the right type, just cast
  • Otherwise use as, like in your second case
  • If you're dealing with a value type, you can either use as with the nullable type (for consistency) or use is and a direct cast

Note that your second form doesn't create a "new intermediate object". It creates a new intermediate variable. So does your first approach really - it's just that the variable is hidden by the compiler, effectively. It's still present in the IL as a stack location, but it doesn't have any representation in source code.

Having said all of that, in this particular case (where you just want to dispose), Darin's approach is the best one, I believe.

Jon Skeet
+1 for suggesting to cast directly. I have seen lots of code where people prematurely handle/omit InvalidCastException/NullReferenceException where in fact it should have been thrown because it actually *is* an error.
bitbonk
Seems like good rules to me. I wasn't aware the the first version also creates a variable behind th scenes, so then it's really a no-brainer and 2) is the way to go.
Øyvind Bråthen
And it was intermediate variable I was thinking, but for some reason it got spelled out as intermeidate object. I know it does not create a new object on the heap, but a new variable on the stack. Thanks a lot Mr. Skeet.
Øyvind Bråthen
+2  A: 

It is regarded as a better solution since a variable allocation is considered faster than a cast. Additionally, the cast can problably be optimized by the compiler.

Either way, this is microoptimizing anyway. Use the code that you think is easiest to maintain.

Andreas Paulsson
+1  A: 

The second alternative is actually somewhat faster. It takes more time to do an extra cast than to access a local varaible.

It isn't slow to allocate local variables. Actually it takes no time at all.

When a method is entered, a stack frame is created by moving the stack pointer down to allocate space for local data in the method. It doesn't take longer to subtract a larger number from the stack pointer, so it takes no time at all to allocate space for local variables. And that is not almost no time, but actually zero time.

The only cost of local variables is that you are using stack space, but as it's only a few bytes out of the one megabyte (IIRC) stack it's no problem unless you are doing deep recursive calls.

Also, the compiler may create local variables on it's own if it's needed, so it's even possible that both methods end up using a local variable in the end.

Guffa
A: 

Would using reflection work here too or would be it be to slow compared to using as?

I created 2 classes, one named ICanDispose which implements IDisposable and another named ICanNotDispose not implementing IDisposable.

In my ICanDispose class I simply show a message box to check reflection has actually invoked the Dispose method.

I have a button on my Form executing the code below. Using reflections it checks if my object implements the IDisposable interface and if so invokes the Dispose method.

private void button1_Click(object sender, EventArgs e)
{
    ICanDispose myObject1 = new ICanDispose();
    ICanNotDispose myObject2 = new ICanNotDispose();

    // Checking object 1 which does implement IDisposable.
    if (myObject1.GetType().GetInterface("IDisposable", true) != null)
    {
        myObject1.GetType().InvokeMember(
            "Dispose", 
            System.Reflection.BindingFlags.Default | System.Reflection.BindingFlags.InvokeMethod, 
            null, 
            Activator.CreateInstance(myObject1.GetType()), 
            null);
    }

    // Checking object 2 which does not implement IDisposable.    
    if (myObject2.GetType().GetInterface("IDisposable", true) != null)
    {
        myObject2.GetType().InvokeMember(
            "Dispose", 
            System.Reflection.BindingFlags.Default | System.Reflection.BindingFlags.InvokeMethod, 
            null, 
            Activator.CreateInstance(myObject2.GetType()), 
            null);
    }
}
François