If there is no difference between two options, choose the one that is easier to debug


In C# you have two ways of casting one object to another. One is to use the as operator, which attempts to casting the object and either returns the successfully cast result or returns null.

Another way is to use the casting operator.

In the case where you are going to use the result of the cast immediately, Which should you use?

// option 1
var thing = GetCurrentItem();
var foo = thing as Foo;
foo.DoSomething();

// option 2
var thing = GetCurrentItem();
var foo = (Foo)thing;
foo.DoSomething();

Now, suppose the thing is not a Foo after all. Both options will crash, but they will crash differently.

In the first version, you will crash with a Null­Reference­Exception at the foo.Do­Something(), and the crash dump will confirm that, yes, the foo variable is null. But the thing might not be in the crash dump. Maybe the crash dump captures only the parameters that participated in the expression that resulted in the exception. Or maybe thing was GC'd away. You can't tell whether the problem is that Get­Current­Item returned null, or that Get­Current­Item returned an object that wasn't a Foo. And if it wasn't a Foo, what was it?

In the second version, there are two ways the code could crash. If the thing is null, then you will get a Null­Reference­Exception at the foo.Do­Something(). But if the thing is the wrong kind of object, then the crash will occur at the point of the cast with an Invalid­Cast­Exception. And if you're lucky, the debugger will show you the thing that could not be cast. Even if it doesn't, you can at least determine from the type of the exception which of the two cases you're in.

Exercise: The following two lines of code are functionally equivalent. Which is easier to debug?

// option 1
collection.FirstOrDefault().DoSomething();

// option 2
collection.First().DoSomething();
Comments (49)
  1. French Guy says:

    If the default value is null (which is the case for reference and nullable types), it’s easier to debug the second line, because an empty collection will cause an InvalidOperationException, while a null first element will cause a NullReference exception (so you know what the problem is). If the default value is something other than null (which is the case for value types such as int), the two lines aren’t functionally equivalent, since an empty collection would result in DoSomething being called on the default value (0 for int).

    1. David Totzke says:

      There could be an extension method on the value type but now you don’t know if the one you are operating with is an actual instance from the list or a default you got from FirstOrDefault(); This is in addition to the other issues you have with Option 1.

      1. French Guy says:

        I don’t see why getting a default value instead of a value from the collection is an issue (assuming you want to use a default value rather than fail if the collection is empty). The real issue is that you have no control over the default value supplied by FirstOrDefault(), so you should use collection.DefaultIfEmpty().First() instead, because that lets you supply your own application-relevant default value.

  2. David Totzke says:

    Exercise:
    Option 1 “FirstOrDefault()” will return null if the collection is empty so you will get a NullReferenceException when you try to call DoSomething(). Not very helpful.
    Option 2 “First()” will result in an InvalidOperationException if collection is empty with the message “Sequence contains no elements” which is easier to debug.

    1. More importantly, if you get a Null­Reference­Exception on Option 1, it’s not clear whether the problem is that collection is null, or that collection is non-null but empty. In Option 2, the two failure modes raise different exceptions.

      1. AberAber says:

        Admittedly, I’ve been trying to harp on this…please don’t chain code…ever, so we can’t tell which is the null reference.

        1. cheong00 says:

          But if it’s okay to not call .DoSomething() if foo is null, I found ?. operator handy.

      2. Ben Voigt says:

        Additionally, collection could be non-null and non-empty but contain a null (in the first element).

        1. Joshua says:

          I made a rule long ago: don’t put nulls in collections.

          1. cheong00 says:

            So do I.

            But seems lots of people, particularly those from Java background, loves to return null as default even for those function returning array.

            So instead of just checking for the length of variable for 0, you also need to check whether the variable itself is null.

  3. Pierre B. says:

    IDK, but if you’re smart enough to choose between two possible ways to code something, which involves knowing that one of the failure modes is the object being null, should you not be smart enough to… check for null in the first place before using the object?

    1. Peter Doubleday says:

      Darn. Your position on this is far more succinct than mine.

      The answer is, Yes.

    2. Ray Koopa says:

      If I see someone using “as” as in the example above, I think the developer didn’t know what “as” is used for in the first place. Don’t be an “as” ass.

    3. Brian says:

      You shouldn’t check for null if it is “impossible” for it to be null; a null check just tells people reading the code that there’s some scenario where it could be null. Otherwise, I agree that you about checking for null (even if only to throw an InvalidArgumentException). Ideally, a NullReferenceException will always indicates a bug in the code that generated a NullReferenceException (see Eric Lippert’s Vexing Exceptions: https://ericlippert.com/2008/09/10/vexing-exceptions/ ).

  4. Peter Doubleday says:

    In terms of the question asked in the post (“which is easier to debug?”), then I agree, use the old-style C cast operator. (Warning: note the implicitly derogatory phrasing there.) So, yes, as an example of how to make the right choice for maintenance programmers — do it this way.

    However, this is imo an anti-pattern in dot-Net terms. The C cast operator style should really only be used internally, in things like ParseInt().

    I suggest that the correct way to handle 99.999% of these situations is to use the as operator and then check for null. You probably shouldn’t need to do this anyway in a “perfect” system, because you shouldn’t need the cast in the first place. But, hey, nobody’s perfect, and it’s a useful mental exercise to think about what “null” means in these circumstances.

    If it means “throw an exception, Bozo,” then, dear Bozo, feel free to throw an exception. Try to make it as specific as possible, though.

    1. kit says:

      Of course, as of C# 7 you can do :
      var thing = GetCurrentItem();
      var foo = thing as Foo ?? throw new Exception(“GetCurrentItem returns something that wasn’t a Foo”);
      foo.DoSomething();

      1. Peter Doubleday says:

        The ?? operator is basically syntactic sugar representing my approach to such an eventuality.
        I’m not knocking syntactic sugar. Syntactic sugar buys eyeballs.

        1. kit says:

          Definitely syntactic sugar, but it lets you get the error handling code out of the way and leaves the good path code much more readable – which has to be good. And anything which reduces the number of keystrokes to write the error handling makes it more likely to get written.

      2. Ken in NH says:

        You could do that before C#7. Even more succinct and readable in 7 though:
        if (!(GetCurrentItem() is Foo var foo)) throw new Exception(“GetCurrentItem returns something that wasn’t a Foo”);
        foo.DoSomething();

    2. If you use as Foo followed by a null test, then you’re saying, “Be careful, because this thing might not be a Foo” which leaves the next programmer scouring the code trying to figure out how a non-Foo could get in there.

      1. Peter Doubleday says:

        I’m not going to claim to be the best C# programmer in the world, Raymond — but with enough experience of dealing with a backlog of 1000+ exceptions caused by lousy type conversions: I will make the bold claim that as and a null check is your friend here.

        1. Peter Doubleday says:

          Basically, the non-Foo got there because the non-Foo was allowed to get to the other side of the managed airtight lock.

          1. I guess the question is what you expect to happen if there is a non-Foo in the collection. If you’re testing for null, it sounds like you don’t want to crash, in which case this entire article does not apply. This article is about the case where you want to crash. In which case, you should crash in a more debuggable way.

    3. SGP says:

      Note that as operator and casting are not equivalent. Option 2 gives thing whatever it is an opportunity for user-defined explicit cast.

    4. Mark S says:

      Why on earth is it an “anti-pattern” to code what you mean? If you’re always expecting the item to be convertible, there is an operation that explicitly does just that…

      `as` means “we might be getting an item that isn’t convertible, and that’s okay too” — if that’s not okay, then why use `as`? Are the parentheses too hard to type? Also as SGP notes, `as` doesn’t consider explicit cast operations defined on the type.

    5. Using “thing as Foo” implicates that your code can react to situation when thing is not a Foo. But in the snippet above the code does not react to such situation, so direct cast is better.

  5. nathan_works says:

    I’d add to “pick the one that’s easier to debug” the extra caveat of “and easier to maintain”.

    One reason why I’m not a big fan of lambdas.

  6. The MAZZTer says:

    In general, I think as should only be used if you expect and want the cast to fail sometimes. In all cases where the cast is expected to always succeed you should probably use the casting operator. Otherwise as just turns a potential runtime error into a later runtime error or even a logic error which is more difficult to debug.

    The compiler will still warn you if you use a nonsensical type with as (eg impossible to be that type so always null) same as if you make a cast that could never work. So no problem there either way, it’s really just a runtime/logic error concern.

    Exercise: If the collection can contain a null, then .FirstOrDefault is the more difficult to debug, since it won’t be clear if the return null is from the collection or not. If the collection is expected to never be empty .First() makes more sense to use, since if it is empty you’ll get an immediate exception which you can debug. Though you may still have to start over and debug when the collection is last manipulated to figure out why it’s empty when it shouldn’t be. So from that perspective neither one of these is “easy” though the second is slightly “easier”.

    1. Peter Doubleday says:

      “In general, I think as should only be used if you expect and want the cast to fail sometimes. In all cases where the cast is expected to always succeed you should probably use the casting operator.”

      Intellectually, yes. Theoretically, yes. In practice (and I speak from experience) a resounding no.

      It all depends upon what you mean by “expects,” I think. Now, first of all, you can’t “expect” the future, which is probably 805 of the coding effort, and will be done by people who don’t necessarily understand your original axioms and business logic.

      And secondly, even if you work with a cohesive team, the chances of your “expectations” being met when you cross the boundaries between GUI and DB and business logic and the network are … the smallest violin that is capable of playing an epsilon you will ever see.

      Do not rely on expectations. Take back control Use the as operator.

      1. cheong00 says:

        I agree with this one.

        Direct casting only for types your code created. If the variable to be casted is from external source (or even a “external team” source), you should use “as” and “TryParse()” as much as possible.

      2. Stuart says:

        I completely disagree. Cast to get a usefully debuggable exception, and have a try-catch somewhere at a high level to handle the problem situation with a friendly error message (and log the exception details or transmit them to your error reporting server).

        Adding null checks after every line of code makes the code harder to read, adds a layer of ambiguity (was the value the wrong type, or was it null already? THE WORLD MAY NEVER KNOW</ominousvoice>) and is easier to mess up when refactoring or when someone else is maintaining the code that’s used to doing things the more normal idiomatic C# way of letting exceptions handle casting errors.

        The best code is self-documenting – the more information you can convey about what’s going on directly in the code itself, the more convenient for the poor sod who has to read your code later (and that poor sod may very well be yourself!). Using “as” vs cast lets me convey to my future self whether I expected the value to always be the type I’m casting to, or to sometimes contain values of other types.

        1. cheong00 says:

          Hey, If “as” or “TryParse()” fails, nothing is preventing you from printing the result of whether the variable is null, and the result of .GetType().ToString().

          Why would you want to throw an exception when you can get the same amount of information without throwing?

          Regarding “Adding null checks after every line of code makes the code harder to read”, ” guess the school is not teaching “IPO”(Input-Process-Output” basic pattern now, and they didn’t teach you that “you should do validation on any received external data you don’t create”? I guess validation on “Input” step don’t harm readability much?

          And the best part is that it’s also self documenting – it explicitly spells out what kind of data do you expect, and in what criteria you’ll say “this shall not pass”.

          1. cheong00 says:

            And IMO this also makes it easier to debug too. In release build your exception don’t get line number. With explicit log message, feel free to add “Validation x failed for variable y – reason” that will explicitly tell you where to look into.

            And even if (God Forbids) you’re using debug builds in production, the line number will be at the place of catch statement, instead of actual line it occurs, if it’s wrapped in try…catch… block.

  7. Jedak says:

    Exercise:
    Option 3: collections.FirstOfDefault()?.DoSomething();

    1. Peter Doubleday says:

      Entirely depends upon what you mean by “default.” Some defaults are more privileged than others in C#. (To provide a concrete example of this, for a random reference type: defaulting to null brings us right back where we were at the top of this post. Defaulting to a null instance — a reference to a (probably static) object that provides default behavior for each and every class method — well, that would obviously be a different thing.

      How dangerous this difference might be depends upon the semantics of your class, or even of your application. Do you want to fail-fast? Use a null. Do you prefer “vanilla,” do-nothing, behavior? Use the null object pattern. As always, the choice is up to you.

      Observationally (over three years), however, I can claim with some confidence that “FirstOrDefault” as a way of hiding any sort of choice between null and not-null is a tad on the not especially safe side. (And you’d be surprised how many experienced programmers ignore the subsequent blue squiggly lines.)

      FirstOrDefault() is a lovely, sweet, beautiful way for Anders to introduce functionaliness into an imperative programming framework. Regrettably, owing to the fact that “null” is not semantically the same as “the empty set,” it doesn’t actually work unless the programmer puts a bit of effort into it.

      1. Jedak says:

        I believe you missed the ?. operator (https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/operators/null-conditional-operators) in my code listing. Which will cause the method to not be called if the previous item before the operator is null. Thus my statement is somewhat of a joke because as written my code listing should never throw an exception. Thus from a debugging perspective you would not be doing any debugging because of an exception.

        1. DWalker07 says:

          Yes, but as Raymond said, you might WANT an exception to be thrown when “collection” is empty.

    2. This is semantically different, because it silently fails (does nothing) when collection is empty.
      The task is aparanetly to fail and do not continue if there is no suitable element in collection.

  8. Wear says:

    The problem with .First() is that it’s just too prone to exploding. Can you really guarantee that the list is never going to be empty? To me that’s not generally an exceptional case. If it is exceptional for a collection to be empty then I’d rather make that check myself and throw an exception with a message or comment saying why it should never be empty.

  9. jnm2 says:

    Yes. Thank you. I see this all the time and it drives me up the wall. Bookmarking so I can link to this.

  10. Vince says:

    If collection is null then you’re screwed either way.

    If it’s not null and a value type collection, then it depends on what DoSomething() does.

    The second one will tell you what’s wrong and usually points you to the line of code, and it works for both reference and value type collections.

    I’ll go with option 2 :)

  11. Jaloopa says:

    Option 2 inside a safety block
    if (collection.Any())
    {
    collection.First().DoSomething();
    }

    1. The “if” version changes the behavior, so it doesn’t fall into the “if there’s no difference between the two options” category.

    2. Aged .Net Guy says:

      In addition to Raymond’s point the guard if(collection.Any()) … is only a valid guard in a single-threaded environment.

      Such environments are already quaint and getting more so daily. IMO an excellent way to fertilize code rot is to be writing assumedly-ST only code in 2017.

      1. Mark S says:

        Plus it’s wasteful, as it allocates another enumerator and starts iterating through it, only to then do the same thing again

  12. Ooh. somewhat of a dilemma. I find “var foo = thing as Foo;” to more clearly indicate what the programmer might want/expect. I dislike casting but the casting is done on it’s own line rather than part of some more complex line.

    The exercise I can’t comment about as I really do not like OOP and try to avoid it (as I’m a procedural kinda guy), they both look kinda messy to me.

  13. Merken says:

    Option 1 will throw a nullref exception when either collection is null or FirstOrDefault returns null (depending on the type of collection, assuming this is a plain object-type array)

    Option 2 will only throw a nullref exception when collection is null.

    So option 2 seems easier to debug to me.

  14. Jonathan Gilbert says:

    I see a lot of comments saying, “as implies this, casts imply that”, but it’s more nuanced than that. If I see code like this:

    var foo = obj as Foo;

    foo.DoSomething();
    Use(foo);

    …then I infer that the author does always expect obj to be a Foo. If they thought that obj might sometimes be a Foo, sometimes not, then they’d have added a check to see if the foo variable got a null reference or not.

    Though I don’t think I’ve ever written it myself, in maintaining other people’s code I’ve also seen casts wrapped in try/catch blocks (apparently people who didn’t know that the as operator existed?):

    Foo foo;

    try
    {
    foo = (Foo)obj;
    }
    catch
    {
    foo = null;
    }

    In this case, the context is very clearly telling me that the author *does* expect the cast to fail sometimes. So, you can’t make conclusions about the expectations of the code or its author based solely on whether the cast operator or as was used.

  15. MgSm88 says:

    I disagree with the premise of this blog post, which is that somehow an `InvalidCastException` or `InvalidOperationException` is more descriptive than a `NullReferenceException`. I think all 3 are terrible options.

    If you are testing an invariant, you should instead *write code that explicitly checks your assumptions, and explicitly throws if those assumptions fail*. And because you’re throwing your own exception, you can make a much more descriptive exception message that helps your future self quickly identify the source of the problem. This way there’s zero ambiguity, and its much faster to find and fix the bug.

Comments are closed.

Skip to main content