22

While reading C# code I found a rather curious snippet:

if( whatever is IDisposable) {
  (whatever as IDisposable).Dispose();
}

I'd rather expect that being done either like this:

if( whatever is IDisposable) { //check
  ((IDisposable)whatever).Dispose(); //cast - won't fail
}

or like this:

IDisposable whateverDisposable = whatever as IDisposable;
if( whateverDisposable != null ) {
   whateverDisposable.Dispose();
}

I mean as is like a cast, but returns null on failure. In the second snippet it can't fail (since there's a is check before).

What's the point in writing code like in the first snippet instead of like in the second or in the third?

Charles
  • 50,943
  • 13
  • 104
  • 142
sharptooth
  • 167,383
  • 100
  • 513
  • 979
  • I guess the latter one leaves a variable in-scope, but I always do it that way anyway. I think I've read a comment from Jon Skeet recently that he prefers the last case too. Although I'd be surprised if they didn't all optimise down to the same thing anyway. – Rup Aug 22 '11 at 11:40
  • 3
    The third option is my preferred one, since you have only one `as` / `is` and a null check - that seems less work/less overhead than having an `is` first and an `as` next... – marc_s Aug 22 '11 at 11:41
  • I've seen last example in a book, seems more logical to me too. – atoMerz Aug 22 '11 at 11:42
  • 1
    I like last one - because of just one cast – Renatas M. Aug 22 '11 at 11:44
  • For some strange reason, people seem to prefer the `as` syntax over the `(cast)` syntax, even if it makes no sense. See also: [Why is the C# "as" operator so popular?](http://stackoverflow.com/q/2139798/87698) – Heinzi Aug 22 '11 at 11:45
  • Note that you can't use the third option when you're testing if an object is a value type (unless you're testing for a nullable type). – cdhowie Aug 22 '11 at 11:45
  • If you use Code Analysis (or fxCop), they will complain about scenarios 1 and 2 and recommend that you refactor as scenario 3. If you look up the reasoning for the rule, Reniuz has hit the nail on the head. It's a performance rule targetted towards avoiding multiple casts. – Toby Aug 22 '11 at 13:00
  • @Heinzi: using `as` makes perfect sense - if your object doesn't match the type, you'll get back a `null` which you can test for easily. If you use the direct cast, your code will blow up..... – marc_s Aug 22 '11 at 14:08
  • @marc_s: Yes, I know. What I meant was: "even *in cases where* it makes no sense". – Heinzi Aug 22 '11 at 14:51

8 Answers8

15

You are correct. The first snippet does not make sense.

From the alternatives, I would recommend the third one, since it is thread-safe in the sense that between the check and the method call, the object cannot be replaced by another one which does not implement the interface. Consider the following scenario with the second snippet:

if (whatever is IDisposable) { //check 
    // <-- here, some other thread changes the value of whatever
    ((IDisposable)whatever).Dispose(); // could fail
}

This can't happen with the third snippet.

Note: There are some subtle differences between a cast and as with respect to user-defined conversions, but they do not apply in this case.

Heinzi
  • 167,459
  • 57
  • 363
  • 519
  • It's easy to make the other thread-safe. `x = whatever; if (x is IDisposable) { ((IDisposable)x).Dispose(); }`, although that's a bit more verbose. – Bart van Heukelom Aug 22 '11 at 12:18
  • I understand snippet 3 is "thread safe" in that nothing can take away or change the newly created reference. However there could be a thread switch just after the null check that performs something with that object through another reference - so the object could be already disposed for instance. Can you explain what you mean by thread safe? – filip-fku Aug 22 '11 at 12:23
  • @filip: Of course. I have clarified the meaning in my answer. – Heinzi Aug 22 '11 at 14:57
3

There is no point of using the as after the is is true, I suspect the code you read or the book is only showing you how to declare and use as and is. I would use it like you suggested in the second snippet through:

IDisposable whateverDisposable = whatever as IDisposable;
if( whateverDisposable != null) 
{
   whateverDisposable.Dispose();
}

Note also that in your first attempt "((IDisposable)whatever)", you are casting the object which will take performance "time to cast", here you are casting twice the first time using as and the second using the explicit cast, so the best way is to implement it using the second method "as IDisposable then check if it null".

Jalal Said
  • 15,906
  • 7
  • 45
  • 68
3

is followed by as is a pointless construct, as you suspect. Ultimately, if you're not sure what type something is or what interfaces it implements, there are two idioms -- one for reference types and one for value types.

When testing for reference types (or a boxed nullable), the last pattern you provide is correct -- as followed by a null test. This will be the fastest approach as the runtime will only have to perform one type test.

When testing for boxed value types, the second pattern you provide is correct -- is followed by a cast.

cdhowie
  • 158,093
  • 24
  • 286
  • 300
2

Agree with the point, but if you really what it to look clean, how about an extension method like such:

public static void DisposeIfNecessary(this object obj)
{
   if (obj != null && obj is IDisposable)
      ((IDisposable)obj).Dispose();
}
Eric Farr
  • 2,683
  • 21
  • 30
1

You are right, The first code you posted is unnecessary. Either use the second if you want the scope of the casted variable to be inside the if or use the third, if it doesn't matter, because it is doing the least amount of work.

Daniel Hilgarth
  • 171,043
  • 40
  • 335
  • 443
1

Also, the last option obviously gives you an in-scope variable that you can reuse later without having to re-invoke a cast.

Strillo
  • 2,952
  • 13
  • 15
1

is followed by as is pointless, beyond being faster to type (because of the keystrokes required for the brackets and how helpful intellisense is with as). Is is marginally slower than both your other examples because the runtime needs to do two type checks.

The CLR has some trickery for as, and as such it should be faster than the is followed by the cast (however, keep in mind that you can't use as with value types).

Jonathan Dickinson
  • 9,050
  • 1
  • 37
  • 60
1

To answer your actual question: Experience and habit.

Before the inclusion of the as keyword in .Net 2.0, the only way to safely determine if an object could be cast to a specific type/interface was with the is keyword.

So, people got in the habit of using is before attempting an explicit cast in order to avoid unnecessary exceptions. This led to the pattern you have second in your list of samples:

if(whatever is IDisposable)  //check
{
  ((IDisposable)whatever).Dispose(); //cast - won't fail
}

Then, we got the safe-cast as keyword. I would guess that most people, when they first started using as, continued to use the familiar pattern, but replaced the direct cast with a safe-cast, and their pattern of choice morphed into your example 1. (I know I did this for awhile.)

if(whatever is IDisposable) 
{
  (whatever as IDisposable).Dispose();
}

Eventually, many or most either realized on their own, or were instructed by fxCop or CodeAnalysis that the 'proper' pattern is your example 3:

IDisposable whateverDisposable = whatever as IDisposable;
if(whateverDisposable != null ) 
{
    whateverDisposable.Dispose();
}

Certainly there are some floating around who are at the example 1 stage still and haven't yet 'evolved' their pattern to your example 3 for some reason, or others who are still just using the good-old time-proven pattern of using is followed by a direct cast.

Toby
  • 7,354
  • 3
  • 25
  • 26