8 Most common mistakes C# developers make

by Pawel Bejger on 7th January 2013

Most Common Programming Mistakes

While working with (young) C# programmers I’ve noticed that some mistakes are being repeated by almost every one of them. These are mostly the mistakes, which once you point them, are quite easy to remember. However, if a developer is not aware of them, they can cause many problems with the efficiency and quality of the developed software. Therefore, I decided to gather the 8 most common mistakes made.

1. String concatenation instead of StringBuilder

String concatenation works in such a way that every time when you add something to a string, a new address in the memory is being allocated. The previous string is copied to a new location with the newly added part. This is inefficient. On the other hand we have StringBuilder which keeps the same position in the memory without performing the copy operation. Thanks to the strings’ appending by means of StringBuilder the process is much more efficient, especially in case of hundreds of append operations.

//INCORRECT
List values = new List(){"This ","is ","Sparta ","!"};
string outputValue = string.Empty;
foreach (var value in values)
{
   outputValue += value;
}

 

//CORRECT
StringBuilder outputValueBuilder = new StringBuilder();
foreach (var value in values)
{
   outputValueBuilder.Append(value);
}

2. LINQ – ‘Where’ with ‘First’ instead of FirstOrDefault

A lot of programmers find a certain set of elements by means of ‘Where’ and then return the first occurrence. This is inappropriate, because the ‘First’ method can be also applied with the ‘Where’ condition. What’s more, it shouldn’t be taken for granted that the value will always be found. If “First” is used when no value is found, an exception will be thrown. Thus, it’s better to use FirstOrDefault instead. When using FirstOrDefault, if no value has been found, the default value for this type will be returned and no exception will be thrown.

//INCORRECT
List numbers = new List(){1,4,5,9,11,15,20,21,25,34,55};
return numbers.Where(x => Fibonacci.IsInFibonacciSequence(x)).First();

 

//PARTLY CORRECT
return numbers.First(x => Fibonacci.IsInFibonacciSequence(x));

 

//CORRECT
return numbers.FirstOrDefault(x => Fibonacci.IsInFibonacciSequence(x));

3. Casting by means of ‘(T)’ instead of ‘as (T)’ when possibly not castable

It’s common that software developers use simple ‘(T)’ casting, instead of ‘as (T)’. And usually it doesn’t have any negative influence because casted objects are always castable. Yet, if there is even a very slight probability that an object can be under some circumstances not castable, „as (T)” casting should be used. See Difference Between C# Cast Syntax and the AS Operators for more details.

//INCORRECT
var woman = (Woman)person;

 

//CORRECT
var woman = person as Woman;

4. Not using mapping for rewriting properties

There are a lot of ready and very powerful C# mappers (e.g. AutoMapper). If a few lines of code are simply connected with rewriting properties, it’s definitely a place for a mapper. Even if some properties aren’t directly copied but some additional logic is performed, using a mapper is still a good choice (mappers enable defining the rules of rewriting properties to a big extend).

5. Incorrect exceptions re-throwing

C# programmers usually forget that when they throw an exception using „throw ex” they loose the stack trace. It is then considerably harder to debug an application and to achieve appropriate log messages. When simply using „throw” no data is lost and the whole exception together with the stack trace can be easily retrieved.

//INCORRECT
try
{
   //some code that can throw exception [...]
}
catch (Exception ex)
{
   //some exception logic [...]
   throw ex;
}

 

//CORRECT
try
{
   //some code that can throw exception [...]
}
catch (Exception ex)
{
   //some exception logic [...]
   throw;
}

6. Not using ‘using’ for objects disposal

Many C# software developers don’t even know that ‘using’ keyword is not only used as a directive for adding namespaces, but also for disposing objects. If you know that a certain object should be disposed after performing some operations, always use the ‘using’ statement to make sure that the object will actually be disposed.

//the below code:
using(SomeDisposableClass someDisposableObject = new SomeDisposableClass())
{
   someDisposableObject.DoTheJob();
}
//does the same as:
SomeDisposableClass someDisposableObject = new SomeDisposableClass();
try
{
   someDisposableObject.DoTheJob();
}
finally
{
   someDisposableObject.Dispose();
}

7. Using ‘foreach’ instead of ‘for’ for anything else than collections

Remember that if you want to iterate through anything that is not a collection (so through e.g. an array), using the ‘for’ loop is much more efficient than using the ‘foreach’ loop. See Foreach vs For Performance for more details.

8. Retrieving or saving data to DB in more than 1 call

This is a very common mistake, especially among junior developers and especially when using ORMs like Entity Framework or NHibernate. Every DB call consumes some amount of time and therefore it’s crucial to decrease the amount of DB calls as much as possible. There are many ways to do so:

  • Using fetching (Eager Loading)
  • Enclosing DB operations in transactions
  • In case of a really complex logic, just moving it to the DB by building a stored procedure

It goes without saying that there are hundreds of other types of mistakes made by C# programmers. If you know any interesting ones or you want to share your opinion about the subject, feel free to leave a comment below! 

If you would like to read more about common mistakes of C# developers read this post.

  • http://www.facebook.com/mysza Michał Michałowski

    As for the first one – it’s not always better to use StringBuilder for string concatentation. It is better if you concat a lot, but using StringBuilder just to replace
    var fullName = firstName + ” ” + lastName;
    is not more efficient.

    Use StringBuilder if you have a really large number of strings to concat, or number of concatentations in unknown at compile time. For very short concats (like the one above), string concatenation is faster and better (StringBuilder is an object that needs to be instantiated – remember that).

    For long concatenations, where you exactly know what you need to concat (like in your example), it’s better to use string.Concat() method (or string.Join() if you need a delimiter between concatenated strings).

    Just my 3 cents ;)

  • http://blog.goyello.com Paweł Olesiejuk

    As a matter of fact the “string” is also a class, therefore it needs to be instantiated. Neverthless you are right. Using a StringBuilder for concatenation first name and last name is like using a hammer for sticking a pin. StringBuilder is not the golden hammer (http://sourcemaking.com/antipatterns/golden-hammer) but it’s good to know about it.

    P.S. For easy concatenation is worth to think about string.Format(“{0} {1}”, firstName, lastName);

  • http://twitter.com/MuigaiMwaura Muigai Mwaura

    1. Could be replaced with one line
        outputValue = string.Join(” “, values);
    It’s also better since you wouldn’t have to remember to leave a trailing space after each item in values.

  • http://www.facebook.com/mysza Michał Michałowski

    I didn’t say string is not a class – just pointed out that StringBuilder also needs to be instantiated. If you concatenate only two strings, single new string is created, so number of newly created objects in both solutions is 1 – no need to use StringBuilder.

    I guess the main problem you wanted to point out in #1 is the fact that those people don’t know that C# string is immutable. If they knew that, they’d look for alternatives and would find StringBuilder and/or various string methods (at least I hope they would).

  • Dalibor Čarapić

    IMHO you should change the title from ‘8 Most common mistakes C# developers make’ to ‘8 things which I think some developers do wrong’. Saying that everything you listed are mistakes which young (assumed inexperienced)  developers makes it look like that whoever makes any of such ‘mistakes’ is young and inexperienced developer. In other words I am sure that you want to help developers with your article but to me it came of a bit arrogant and condescending. My thoughts on your points:

    1. Already answered by other comments, you can use String.Join to make it more readable (and probably just as efficient)
    2. If you are not aware of FirstOrDefault then Where/First is just as good. IMHO not knowing that there is a method in the framework is not a mistake. There are hundreds/thousands of nice methods inside .NET framework for which any developer could say ‘I wish I knew that before’.
    3. I agree that every developer should know the difference between ‘(T)x’ and ‘x as T’
    4. I guess that by ‘rewriting’ you mean copying values from/to properties. This is very context related ‘mistake’ – if you do not use DTOs then there is not that many places where tools like AutoMapper are  helpful. I could also say something like ‘you should always use OR mapper if you are dealing with the database’ but I would not say it is a mistake if someone does not use it (because its not a mistake).
    5. I agree that every developer should know about exception re-throwing
    6. I agree that every developer should know about what some basic keywords (such as ‘using’) do
    7. That is just a micro-optimization and if you are going that way why not include collections as well (http://codebetter.com/patricksmacchia/2008/11/19/an-easy-and-efficient-way-to-improve-net-code-performances/). I prefer foreach in almost all scenarios except in some specific cases (such as doing reverse for loop through collection in order to remove values).
    8. Same as 4, depends on the application and the context.

  • Pingback: The Morning Brew - Chris Alcock » The Morning Brew #1268

  • http://twitter.com/NathanGloyn Nathan Gloyn

    using the as (T) will not always work how you would expect, check out Eric Lippert’s post on the matter http://blogs.msdn.com/b/ericlippert/archive/2009/10/08/what-s-the-difference-between-as-and-cast-operators.aspx

  • Kevin Watkins

    The article you use to support point 7 is 9 years old – the C# and JIT compiler has changed a lot since then! Can you back up your point with a more recent example? (Of course as Dalibor points out it’s a micro-optimisation anyways so can hardly be considered a ‘mistake’)

  • B BL

    2. FirstOrDefault has its place, but in your example with integers using FirstOrDefault is a bad idea: suppose your list doesn’t contain any fibonacci numbers. Then the function will return the default value of zero and the caller may act on the wrong premise that zero IS a fibonacci number. And throwing an exception constitues a strong contract with the caller, so it’s definitely better in this case.

    7.  for is almost never faster than foreach. Even in cases when it is, the difference is negligible in majority of practical applications. BTW, “not a collection, e.g. an array” is somewhat sloppy and vague definition.

    8. Very debatable, if you are talking about relational sql DBs. In many scenarios the opposite is true: manipulating data in multiple calls results in better perfromance. It allows to shorten the transaction duration that minimizes blocking, and allows to use simpler queries increasing the chance that the DB engine will use an optimal execution plan.

  • http://dcstraw.myopenid.com/ Dave

    Using ‘as’ definitely has its place and I do so quite often, but if the code following the type check requires that the cast succeed, then having the code throw an InvalidCastException can be desirable. It’s certainly much better than using ‘as’, then not checking for null and throwing a NullReferenceException.

  • Craig Wagner

    I was going to go into detail about the problems with this article but previous posters have already pointed out all the problems I noticed. I just want to add that I feel like articles like this are dangerous. While points 5 and 6 are, in my opinion, absolutely best practices that should be followed in 99% of cases the rest of the items are dependent on what you are doing, how much data you are working with, and a host of other considerations. To tell a developer “always use for instead of foreach” will generate a bunch of developers who don’t understand why other than they read it on some blog.

  • http://profile.yahoo.com/NGWSEUWJRIUQAQBHOF6KQJUN6I B. Clay Shannon

    Thanks – good list.

  • Vaxman2

    Step 5 – The sentence should be “they lose the stack trace” not “they loose the stack trace”.

  • Ross

    I completely disagree with the First vs FirstOrDefault issue. You should only use FirstOrDefault if you know it’s possible that you won’t find a match and that you’re going to handle things differently. Otherwise, if you always expect there to be at least one match, then you should use First() and let the exception be thrown. It’s much easier to debug First() throwing an exception than random null object exceptions.

  • Pingback: 8 Most common mistakes C# developers make | Hispafoxing

  • Samuel Langlois

    Good article although point 7 is just wrong. There is almost no performance difference between for and foreach, even for arrays, and foreach is more readable (unless you need to do something with the index), so it’s almost always better to use foreach. Also, how is array NOT a collection?

  • http://www.Marisic.Net/ dotnetchris

    1. You seem to be repeating mostly misinformation. In the exact scenario you described, your first  code sample would actually likely be extremely more performant. Also this type of code would nearly never likely exist. More realistically it would be something like

     outputValue = a + b + c + d;

    The compiler and string interning is pretty magically when it comes to this code.

    As others noted, the real magic string method is string.Join.

    string.Join is unequivocally the best method. It is the fastest overall, faster than everything else.

    The only reason to use stringbuilder is for code readability and also using AppendFormat possibly mixed in with Appends. If you only need .Append() you should be using string.Join.

    If you’re getting into strings that have thousands or millions of characters then you should be using textwriters and stuff like that as opposed to requiring huge stack allocations.

  • http://twitter.com/NOtherDev NOtherDev

    Agreed with Dalibor and others. Most of the points here are very subjective and, as always, there are a lot of scenarios when completely different advices can be given. As always, the most correct advice is “it depends”.

    I’ve commented a bit broader on my blog –
    9th most common mistake – using “always” and “never”

  • Pingback: NOtherDev

  • http://profiles.google.com/grauenwolf Jonathan Allen

    > 3. Casting by means of ‘(T)’ instead of ‘as (T)’ when possibly not castable

    So instead of a type cast exception telling you exactly what went wrong you are going to inject a null reference exception at a later date? That’s not an improvement, that’s a landmine.

    The ONLY time an “as T” cast is acceptable is if it is immediately followed by a null check.

  • http://profiles.google.com/grauenwolf Jonathan Allen

    > When using FirstOrDefault, if no value has been found, the default value for this type will be returned and no exception will be thrown.

    That is not a good thing. That is sloppy programming done by people who would rather paper over problems instead of actually fixing them.

    The developers down the road are going to hate you when they find out the reason their code was silently failing.

  • http://profiles.google.com/grauenwolf Jonathan Allen

    > String concatenation instead of StringBuilder

    You got that one wrong too, but at least you get half-credit.

    You should be using String.Join so that it can pre-allocate a buffer of the correct size.

  • http://www.facebook.com/junior.painkiller.5 Junior Painkiller

    I use FirstOrDefault; the next line is 

    If (varObject != null )

    Which is proper checking. How is it sloppy programming to run a linq query that might yield no results?

  • http://twitter.com/cpmcgrath Chris McGrath

    I actually disagree with a lot of these
    1. Yes you should use StringBuilder for long or indeterminate number of joins, but it can also overused. When dealing with very short number of strings it’s actually faster to just concat the strings (The benefit of StringBuilder doesn’t outweigh the cost of setting it up). And for the next few the performance is so minimally better it makes no difference. And StringBuilder does have one disadvantage, it’s uglier.

    2. I agree with using .First(x => …) instead of .Where(x => …).First() but the decision to use First or FirstOrDefault is a completely different discussion. If you are going to assume that it was found (so you don’t check if it was afterwards) FirstOrDefault will (assuming it’s nullable) make you throw a NullReferenceException. Using First will return a more meaningful error.

    3. I’m really sick of people claiming this. (T)obj is so much better 90% of the time. Like before, as leads to meaningless null reference exceptions possibly nowhere near the problem is. While (T) throws a meaningful InvalidCast where the problem happens.
    But people say, “Oh but you should immediately say “if (x == null)” which prevents that, but people don’t always do it, they think, “It will always work”. And let’s not forget you can say if (x is T) on old style casts.
    As is meant to perform better, but I doubt you could ever find a situation where it’s actually noticeable and the dangers aren’t worth it.

    7. The article is pretty old, so I don’t know if that’s true anymore, but foreach reads nicer and allows for better abstraction.

  • http://twitter.com/cpmcgrath Chris McGrath

     Personally I use the rule that there is only 2 cases where you should use an as…

    1. When returning a value from a property
    2. When used with a coalesce – var str = obj as string ?? string.Empty;

    Personally, I’d love it if you were forced to put a coalesce with as, if you really wanted null you could say
    obj as string ?? null;
    and it could lead to being able to use it with primatives -
    obj as int ?? -1;

  • Pingback: 8 Most common mistakes C# developers make « The code is art

  • Rémi BOURGAREL

    The comments in the Foreach vs For article are all I think about this one.

  • http://www.facebook.com/people/Paweł-Bejger/100001094234479 Paweł Bejger

    First of all sorry for the late response to your comment – unfortunately I did not
    have time to do this earlier. Secondly I really appreciate that you found interest in my
    post and that you found time to share your views and opinions about it.

    Below I have put my answers to your points:

    1.The general thought here was that StringBuilder is better
    than simple string concatenation when we have multiple append operations. By “multiple”
    I meant “a lot” but probably I should have put more emphasis on the fact that I
    am talking about large numbers of elements.

    There are obviously other alternatives as well like
    string.join, that’s fully true. What’s important though is not to use (due to
    the lack of knowledge or laziness) simple string concatenations when we have large
    numbers of append operations.

    2. That’s true that there are hundreds other methods in .NET
    framework as well. Yet there are some methods that C# developers use quite
    often, therefore it is relevant that they understand that there might be some
    alternatives to what they currently use.

    What I have meant with suggesting the usage of
    FirstOrDefault are the situations when it’s possible that null will be returned
    and it should not make the application crashes. If course it could be handled
    by means of exceptions but I prefer if exceptions handle errors and not just
    some exceptional paths. I think we should not abuse exceptions. 
     

    4.The problem that I found with rewriting properties
    (especially if you have an architecture that distinguishes between entities and
    business models) is that 1) it’s a lot of writing, 2) it’s easy to ommit some
    property that you want to rewrite from one object to another. Generally it’s
    not necessarily a problem when you have only a few properties, but it might be
    when you have many of them. So let’s say you have an entity Person and a
    business model PersonModel. PersonModel is enriched with some additional
    properties like FullName, Age, etc. but the rest, let’s say, 10 properties
    should be simply copied. In this case you can either rewrite the properties
    line by line or use some mapper which makes it in my opinion much cleaner.

    7.What I meant here is not to blindly use foreach. There are
    some developers who forget that for-loop exist at all though there are indeed
    some scenarios when the usage of for-loop is preferred.

    8.Can you provide me with the context when having more than
    1 call to a DB is a better solution? 

  • http://www.facebook.com/profile.php?id=604406782 Anthony Bodineau

    In Point 3 you’re suggesting to use as (T) instead of (T) but the article you’re referencing is suggesting the opposite…

    http://gen5.info/q/2008/06/13/prefix-casting-versus-as-casting-in-c/
    ” Prefix-cast should be the default cast operator on your fingertips”

  • rtpHarry

    I also prefer to use strong.Format() for simple text formatting. I feel it separates the string from the variables nicely and I don’t loose any sleep over the possible microseconds I could be loosing performance-wise.

  • http://narayana-games.net/ jashan

    Number 9: Thinking that performance is everything. You should remember: “Premature optimization is the root of all evil.” (Donald Knuth)

  • Claudio Estrugo

    In many cases, foreach makes the code a lot more clear to understand that a simple for. And you’re refering to a 9-year-old article about compiler performance. I bet a lot has changed since then.

  • Kirk Wood

    Number 7 is a classic example of premature optimization. Which leads me to believe that you have fallen for the most common programming error throughout most any language:

    Not writing for maintenance.

  • http://twitter.com/mike_girkin Mike V. Girkin

    I agree with all the points, except 7. It is a classical example of “Premature optimization”. I think you should put this mistake instead of 7.

  • http://www.goyello.com Pawel Bejger

    Thanks for reading the post and sharing your thoughts, I really appreciate this.
    What I have meant in fact with suggesting the usage of FirstOrDefault are the situations when it’s possible that null will be returned
    and it should not make the application crashes. If course it could be handled by means of exceptions but I prefer if exceptions handle errors and not just
    some exceptional paths. I think we should not abuse exceptions.But you are right that I should have explained it better in the post.

  • http://twitter.com/KarolinaDryja KarolinaDryja

    Thanks for the post!

  • Bill Kempf

    1. As pointed out, StringBuilder is NOT always better. Blindly using StringBuilder is as much a mistake as blinding concating your strings.
    2. Dropping the Where in favor of the lambda predicate on First/FirstOrDefault I’ll agree with. The absolute about using FirstOrDefault I won’t. As pointed out your example is one scenario where I absolutely cannot agree. If you post “newbie mistakes” type articles, you’d best be very sure your examples are correct and thoroughly explained, because newbies can’t figure out when you blunder so badly.
    3. The description here was pretty horrible. What does it mean to be “possibly not castable”? As others have pointed out, what you really mean is you can in some way handle the cast failing. Otherwise, the strict cast that throws an exception IS the correct way to code this.
    4. Mapping is something that isn’t all that common, and in many cases a mapping framework is extreme overkill and a likely cause of performance issues. It’s a tool you should be aware of (though I’d debate whether or not newbies should be aware of them), but not using it is hardly a mistake.
    7. You’re advocating a micro-optimization that’s unlikely to actually make any difference in the overall performance but is much more likely to result in bugs (off by one errors, etc.), especially by newbies. Probably the worst advice in this article.

  • Piotr Pawłowski

     I fully agree :)

  • sebwells

    there are instances where using First is correct and there are instances where using FirstOrDefault is correct, neither is generally more correct than the other.

  • Robert LeVan

    Good post Pawel.  While one or 2 points might be debated under particular circumstances, there’s one thing I think is dead wrong. “In case of a really complex logic, just moving it to the DB layer by building a stored procedure” implies that embedded SQL is a good idea.  It’s not.  Unless it executes every time with the same predicate and parameter values, it will compile every time it hits the DB server.  This takes a finite amount of time that is eliminated if you put even your simple SQL into a stored procedure.  Even better, as a stored procedure, unless you wrote it to recompile every time it runs (necessary when there is a wide variation in the rowcounts of different result sets, 1.e., 100 one time and 100,000 the next), the execution plan will stay loaded for as long as there is available memory and that page is not needed by a new process, thus gaining even more efficiency.

    This may not make much difference if you have a service that consumes a few hundred or a few thousand requests per hour, but scale that up to a few hundred thousand per hour and tens of millions per day and now you are talking about significant time if your SQL is embedded vs. a stored proc.

    Moreover, a good developer would use the same TRY…CATCH logic in the stored proc as he would use in the application to capture and store the errors at the source, rather than in the application log.

  • http://groggyjava.myopenid.com/ ken ehrman

    // EVEN
    BETTER

    try

    {

       //doing something specific with SomeValue

    }

    catch (Exception
    ex)

    {

       //some exception logic [...]

       throw new Exception(

          string.Format(“FAIL!
    HERES’S SOME VALUE TO HELP FIND OUT WHY: {0}”,  

                        SomeValue),

          ex);

    }

    // gives you
    // 1.) nested stack trace, 
    // 2.) something to tell the client
    // 3.) something to duplicate/fix the issue

    // call it defensive programming if you must,
    // but truth is computations generally don’t fail,
    // what fails is another part of the system out of your control
    // writes data incorrectly or inconsistently// or you’re uncovering a heretofore unseen edge case

  • Mykhailo Rybnikov

    Though, as a .Net developer, you might be much more interested in clean and easier to read code more, than in several microseconds.

  • Pingback: Re: 8 Most common mistakes C# developers make | Zeckul

  • André Slupik

    I agree with most of the points you made, however some are misleading or downright incorrect. As this would require a lengthy post, I’ve decided to put the answer on my blog instead, for whoever’s interested: http://zeckul.wordpress.com/2013/01/09/re-8-most-common-mistakes-c-developers-make/

  • http://twitter.com/ToreSenneseth Tore Senneseth

    Unless MS has changed the implementation of string.Format in .NET 4.5, it uses StringBuilder internally, so string.Format is actually just “syntactic sugar” for using StringBuilder.

  • Asoka Sampath Edussooriya

    great article

  • Dan Sutton

    Interesting. I’m not sure I agree with all of it, but it’s definitely thought-provoking. I think what really needs to happen with C# (and please tell me if such a thing already exists) is for there to be some kind of “bible” which tells us what’s going on internally when a lot of the library structures are used – especially things in System.Collections.* – and how many of the loops and so forth (not to mention LINQ) are compiled. I think such a reference would help programmers decide which methods to use when writing code, so that it becomes more efficient.

  • http://www.facebook.com/bwallan Brian W. Allan

    Thanks!

  • http://www.facebook.com/people/Paweł-Bejger/100001094234479 Paweł Bejger

    Robert – thanks for complimenting my posts and for sharing your thoughts. There is though one slight misunderstanding of what you have interpreted from my blog post: I have used the term DB layer (so not the data access layer- DAL  or data logic layer- DLL, but database layer) and by this I meant in fact the database itself. Probably I should have written simply “stored procedure in a DB” to avoid any misunderstandings.  Anyway – what you wrote about the difference in having SQL logic in DAL/DLL and DB is a very interesting subject and I think that you could write a nice blog post out of this :D

  • http://www.facebook.com/people/Paweł-Bejger/100001094234479 Paweł Bejger

    Thanks for reading my post and sharing your thoughts. You are fully right that there are other alternatives to a simple string concatenation as well (like string.join or string.format). Comparing all these alternatives is a subject for a separate post (and many people already wrote such posts) and its usually very subjective to a certain context. What I wanted to achieve when creating this point of my post was to simply make developers aware that they should not blindly use ‘+’ operators because it’s not applicable in case when there are lots of append operations. Differences in performance between such alternatives of StringBuilder like string.join or string format (which is indeed based inside on StringBuilder) are very tiny and usually depending on the context and it was not the goal which I wanted to achieve when writing down this point.