Cast then check or check then cast?

Question

Possible Duplicate:
Casting vs using the ‘as’ keyword in the CLR

Which method is regarded as best practice?

Cast first?

public string Describe(ICola cola)
{
    var coke = cola as CocaCola;
    if (coke != null)
    {
        string result;
        // some unique coca-cola only code here.
        return result;
    }
    var pepsi = cola as Pepsi;
    if (pepsi != null)
    {
        string result;
        // some unique pepsi only code here.
        return result;
    }
}

Or should I check first, cast later?

public string Describe(ICola cola)
{
    if (cola is CocaCola)
    {
        var coke = (CocaCola) cola;
        string result;
        // some unique coca-cola only code here.
        return result;
    }
    if (cola is Pepsi)
    {
        var pepsi = (Pepsi) cola;
        string result;
        // some unique pepsi only code here.
        return result;
    }
}

Can you see any other way to do this?

See the perfect answer http://stackoverflow.com/a/496167/52277 for similar question — Michael Freidgeim, May 30 '12 at 12:43

score 20 · Accepted Answer · edited Mar 31 '10 at 05:59

If the object may or may not be of the type you want then the as operator (your first method) is better in two ways:

Readability and ease of maintenance: you are only specifying the type once
Performance: you are only performing the cast once, instead of twice. (Trivia: when you use the is keyword, the C# compiler internally translates it to as, ie. coke is Cola is equivalent to (coke as Cola) != null)

If the object should always be of the requested type then just do (Coke)cola and let it throw an exception if that's not the case.

score 7 · Answer 2 · answered Mar 31 '10 at 05:13

7

The first (casting first via as) is slightly more efficient, so in that regard, it ~might~ be a best practice.

However, the code above, in general, displays a bit of "code smell". I'd consider refactoring any code that follows this pattern, if possible. Have ICola provide a describe method, and let it describe itself. This avoids the type checks and duplicated code...

answered Mar 31 '10 at 05:13

Reed Copsey

554,122
78
1,158
1,373

Agree about the code smell - from the example this looks like a problem that could be better solved by polymorphism rather than by wildly casting objects about. – Timo Geusch Mar 31 '10 at 05:59
I agree with the code smell. In this case however, I don't want methods in my view permeating my Cola objects. – jamesrom Mar 31 '10 at 23:06
1

@jamesrom: Well, I did try to answer your question, first, before mentioning that (as is faster). However, I would really think about how you could eliminate the duplicated code- can your View pass in a delegate to handle the "specific" code more efficiently, maybe? – Reed Copsey Mar 31 '10 at 23:09
I came here to learn about similar problems I was having. I do not understand how to do this `ICola` code sniffing, and I don't know what to google search on. What is this called, and how would one implement it? – Jun 20 '12 at 18:33

score 4 · Answer 3 · answered Mar 31 '10 at 05:32

4

This example uses a local parameter which is safe, but many times the type check is applied to fields (member variables) of a class. In which case "as"-then-check is safe, but "is"-then-cast creates a gratuitous race condition.

answered Mar 31 '10 at 05:32

Ben Voigt

277,958
43
419
720

score 3 · Answer 4 · answered Mar 31 '10 at 05:17

3

I think it's more efficient 1st way: Cast and then check, but...

Lots of time you develop for developers, and in my opinion, it's much more readable checking first and then casting...

answered Mar 31 '10 at 05:17

Oscar Mederos

29,016
22
84
124

score 1 · Answer 5 · answered Mar 31 '10 at 06:10

1

Let me just put it out there. But I think neither is right :) In your particular example, why have an interface at all then? I would put a "Describe" method on your ICola interface, then implement the describe logic in your CocaCola and Pepsi classes that implement the interface.

So basically put that // some unique <some cola> only code here. into the implementing classes.

But to answer your question, I think check-then-cast is more appropriate.

answered Mar 31 '10 at 06:10

Strelok

50,229
9
102
115

That is what I would do if I could. But say, for example, that I didn't want my view logic permeating my Cola objects. – jamesrom Mar 31 '10 at 23:12
In that case, use some decorator or visitor-based solution for example, anything is better than manually inspecting the runtime-type of objects. I even think view logic would be less evil than casting and switching on types all over the place. – sara Feb 05 '16 at 10:07

Cast then check or check then cast?

5 Answers5

Linked

Related