Cast then check or check then cast?

Question

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?

Answer 1

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...

Answer 2

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.

Answer 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...

Answer 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.

Answer 5

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.