CODEWITHSUNDEEP
c#.net
Øyvind Bråthen
Øyvind Bråthen

Reputation: 60744

Cast using is twice, or use as once and create a new variable

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?

Upvotes: 6

Views: 1143

Answers (6)

akjoshi
akjoshi

Reputation: 15802

No one mentioned here that FxCop shows a DoNotCastUnnecessarily warning in case of first approch. Help for this rule suggests the second approach in place of first

http://msdn.microsoft.com/en-us/library/ms182271(VS.90).aspx

Duplicate casts degrade performance, especially when the casts are performed in compact iteration statements. For explicit duplicate cast operations, store the result of the cast in a local variable and use the local variable instead of the duplicate cast operations.

If the C# is operator is used to test whether the cast will succeed before the actual cast is performed, consider testing the result of the as operator instead. This provides the same functionality without the implicit cast operation performed by the is operator.

Upvotes: 1

Nope
Nope

Reputation: 22339

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);
    }
}

Upvotes: 1

Guffa
Guffa

Reputation: 700840

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.

Upvotes: 2

Andreas Paulsson
Andreas Paulsson

Reputation: 7813

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.

Upvotes: 4

Jon Skeet
Jon Skeet

Reputation: 1503839

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.

Upvotes: 12

Darin Dimitrov
Darin Dimitrov

Reputation: 1039498

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.

Upvotes: 13

Related Questions