Monthly Archives: January 2013

Re: 8 Most common mistakes C# developers make

A recent blog article by Pawel Bejger lists some common mistakes made by C# developers. As noted by several in the comments, some of this information can be misleading or incorrect. Let’s review the problematic points:

String concatenation instead of StringBuilder

The author argues that using string concatenation is inefficient compared to using StringBuilder. While this is true in the general case, including the example presented, this must be taken with a grain of salt. For instance, if you just want to concatenate a list of strings as in the example presented, the easiest and fastest way is String.Concat(), or String.Join() if you need to insert something in between each pair of strings.

In addition, compile-time concatenations are automatically translated by the compiler into the appropriate calls to String.Concat(). Therefore, the use of StringBuilder should be reserved to building complex strings at runtime; don’t go blindly replacing every string concatenation code with StringBuilders.

Casting with “(T)” instead of “as (T)”

The author argues that if there’s the slightest probability a cast could fail, “as (T)” should be used instead of the regular cast. This is a very common myth and is misleading advice. First, let’s review what each cast does:

  • “(T)” works with both value and reference types. It casts the object to T, and throws an InvalidCastException if the cast isn’t valid.
  • “as (T)” works only with reference types. It returns the object casted to T if it succeeds, and null if the cast isn’t valid.

Each cast expresses a different intent. “(T)” means that you fully expect this cast to succeed, and that if it doesn’t, this is an error in the code. This is the simple and general case. “as (T)” means that you fully expect this cast NOT to succeed at least some of the time, that this is a normal occurrence, and that you will take care of manually handling it via a null check after.

The real mistake I often see is “as (T)” not followed by a null check. The developer fully expects the cast to succeed so doesn’t bother to write the null check, but the day something goes awry, no exception is thrown on the invalid cast, no null check is performed, and a hard to track bug has found its way into the code base. So my advice is to always use the regular cast “(T)” unless you intend to check yourself for the invalid cast via “as (T)” and a null check.

Using “foreach” instead of “for” for anything else than collections

The author claims that using “for” to iterate “anything that is not a collection (so through e.g. an array)””, the “for” loop is much more efficient than the “foreach” loop.

As with any claims regarding performance, this is easily verified. In this case, the verification proves the author wrong.

As the page he links to claims (and which is very outdated), the generated IL for a “foreach” loop is slightly larger than that for a “for” loop. This is no indication of its performance however, as the speed of code doesn’t correlate with its size, and IL isn’t actually executed but compiled just-in-time to assembly code, where further optimizations may happen. A very simple test performing the sum of an array of ints shows that the machine code generated for a foreach loop is in fact shorter, and the execution time also shorter, than for the for loop. Results on my machine (x86 release mode with optimisations, no debugger attached):

SumForEach: 482 ms

SumFor: 503ms

As you can see, this is in fact a very small difference, so I would advise that in general you use the loop that is the most idiomatic or readable, and not worry too much about the performance difference. But if you’re writing performance-sensitive code, don’t rely on someone else’s benchmark and test things out yourself. You may find that it’s not because ┬ácode is visually more compact or more C-like that it performs any better.

Advertisements