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/people/Paweł-Bejger/100001094234479 Paweł Bejger

    Thanks for reading my post and sharing your thought regarding point 7. I fully see your point, but I think you overinterpret the meaning of Premature Optimization. Prematura Optimization says about not spending hours on thinking out how we can tune performance upfront, but doing this only when some bottlenecks will be found. But it does not give a right not to focus on performance at all in the earlier phases, especially when it does not cost much from a developer.  There is principle called Design For Performance, which in fact says, shortly speaking, about not abusing Premature Optimization. If there are some code practices which do not require big effort from developer’ perspective (or are just a matter of some habit of a developer), but can influence performance then they should be used. You can read more about Design For Performance principle here: 
    http://c2.com/cgi/wiki?DesignForPerformance
    And about the comparison between these two: 
    http://blogs.msdn.com/b/ericgu/archive/2005/03/23/401057.aspx

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

    Mike- thanks for reading my post and sharing your thoughts. I have already responded to the point you made by answering to Kirk Wood, who also mentioned the Premature Optimization issue in his comment. 

  • http://www.facebook.com/people/Григорий-Перепечко/100000058007457 Григорий Перепечко

    Remove the post. To many wrong ideas for such a small post.

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

    It’s true that string.join is an alternative for StringBuilder (and there are many other alternatives which I could write about if it was a post focused purely on the comparison between different string append possibilities). 

    What is though not fully true is that string.join is better than StringBuilder in all possible cases. 

    When you do a string.join, the runtime has to: 1: Allocate memory for the resulting string 2: copy the contents of the first string to the beginning of the output string 3: copy the contents of the second string to the end of the output string.If you do two joins, it has to copy the data twice, and so on.StringBuilder allocates one buffer with space to spare, so data can be appended without having to copy the original string. As there is space left over in the buffer, the appended string can be written into the buffer directly. Then it just has to copy the entire string once, at the end.

    Therefore whether to use StringBuilder, string.join, string.format or any other alternative is a good subject for a separate post. What is though true (and it was my point when writing about this) is that developers should not use “+” operator for appending string in case of big amounts of strings to concatenate. Probably, and here I agree, I should have listed down all the most popular alternatives instead of refraining myself to StringBuilder. 

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

    Thanks for reading my post and for pointing out this typo :)

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

    Thanks for reading my post and for writing down this famous quote. I fully agree that we should remember about the fact that “Premature optimization is the root of all evil”, but I would also add a sentence “Don’t always excuse yourself by abusing Premature optimization and start thinking about the Design For Performance principle as well”. What I mean here is that I have no doubts that Premature optimization is a very good principle, but it’s applicable mostly to the situations when it requires a lot of energy and time of a developer to optimize the system before it’s really needed (before bottlenecks were actually found). Design For Performance says about having a certain set of good code practices to keep code optimized “out of the box”. Such practices once learnt, do not require a developer to sit hours on optimizing the solution, they are just done automatically and “eat” a similar amount of time as the normal development would take.

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

    Thanks for sharing your point of view. Some of your thoughts have been already answered by me in replies to other comments, so, trying not to break the DRY rule, I will write about the point that have not been mentioned in other comments:

    You wrote: “not a collection, e.g. an array” is somewhat sloppy and vague definition.

    My answer: You are right that an array might also be treated as a collection of objects. But because it’s a post for C# developers and in C# language Collections (System.Collections) and Arrays are two different concepts, I made such a division. 
    You can read more about the differences between Collections and Arrays in C# in this article: 
    http://msdn.microsoft.com/en-us/library/9ct4ey7x(v=vs.90).aspx

  • Ben

    I have a question about string concatenation…can we get around the boxing problem by using

    string x = string.Concat(a,b,c);

    Always wanted to know, but couldn’t find the answer.

  • http://www.facebook.com/profile.php?id=603952213 Richard Saulnier

    This is inefficient? … Dude .. code C++

  • Pingback: Weekly Tech and Science Links (#3) | Darker Nemesis

  • Pingback: Liens de la semaine #12 | frenchcoding

  • Pingback: Performance of foreach vs classic for in C# – n00b coder

  • Pingback: Last Read (Jan 2013) « Yordan Dimitrov

  • Pingback: 8 Most common mistakes C# developers make | TNVBalaji.com

  • Pingback: 8 Most common mistakes C# developers make | ChangSu's tech blog

  • Pingback: Линкблог #11

  • http://www.facebook.com/thanasis.ioannidis Θανάσης Ιωαννίδης

    Concering the performance difference between for and foreach, it all depends in each particular case. Foreach is not used on collections. It is used on Enumerables. And that’s different. An enumerable may not be eligible for use with the for statement at all. Moreover an enumerable’s items may not exist as sequential elements in memory, or may not exist at memory at all prior to their first access. An enumerable may create objects “on the fly”, as you request them.

    A “for” loop may (or may not) be slightly faster in case of arrays, but as a .NET developer (meaning, a developer on a managed language) gaining a few milliseconds here and there is not crucial. Optimizing the program in the technique or algorithm level will be extremely more effective than optimizing it in the IL. One of the most common optimizations I do, is using Dictionaries, instead of just Collections, where keyed access to objects is needed. It certainly takes some time to build an index over your data using a dictionary [actually O(n) time], but saves extremelly more time later when you access the data, especially when the data are cross referencing each other by their key.

    I could say, not using index on keyed data, and using just collections, is worse than using a foreach instead of a for.

  • http://www.facebook.com/thanasis.ioannidis Θανάσης Ιωαννίδης

    Also, the difference between (T)Val and Val as T, again is not about choosing what is best. It’s all about choosing what fits best to what you want to do.

    Val as T will return null if Val is not castable to T. (Τ)Val will just throw an exception.

    If it is within your program’s logic to expect a Val that is not castable to T, and you want to perform certain actions in such a case then Val as T is a good choice. You check the result and compare it to null and you do whatever you want to do.

    On the other hand, if you always expect a value that is castable to T, then (T)Val MAY be better choice. If you get an exception there it is a good indicator that something has gone wrong with your program that needs to be fixed.

  • knakker

    Quote: “That is sloppy programming done by people who would rather paper over problems instead of actually fixing them”….

    I understand your remark that hiding possible null reference execptions is wrong, and giving you as a programmer a hard time in you day to day maintenenace chores.

    However, as much as you like production code to be your playground, I wonder how you would react when for example your local ATM machine thows you an exception instead of dishing out your money. Nobody was papering over anything here, but I bet you would be really annoyed if this would happen in real life!

    This highlights the real core of the “sloppyness”. With the rush to market mindset there’s almost no considereration what a program (or method) really should do. Discussions bloated with oneliners and one sided remarks do not really contribute in highlighting the real problems.

    I’m talking about pre- and posconditions, and invariants. The real sloppyness can be found here.

    It is not a question if a linq query should or should not contain a First i.s.o a FirstOrDefault, but whether the returned data is allowed to contain a null value in the first place.

    When in does, then obviously you should have an explicit scenario coded.

    If it isn’t allowed, simply blowing up and let the linq query throw isn’t a complete solution either. You still have to write additional code to properly deal with this situation.

    Dealing with the situation in a gracefull way, and defining how an application or usecase should proceed is more than often overlooked, or plastered in ad an afterthought.

  • Florian Jackson

    Hey Pawel, thank you for this interesting post! Even though I think I’m an experienced C# developer I found some good hints in your Top 8 ;) Do you think you could do another issue with the followups? “8 more things C# developers should think about” ;)

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

    Hi Florian – thanks for reading my post and the words of appreciation :) I think that your idea is indeed very good, because there are many other things that could be mentioned by me as well. I will definitely work on a followup post in the nearby future.

  • Pingback: Some links each developer should click on it – Feb 2013 « Marcin Dembowski

  • Bruno Brant

    Which is the same as with foreach vs for. Yes, for may be more faster, but foreach states the intention of the code way more clearer than a for.

  • Bruno Brant

    One comment: #3 is a bit misguided. I’ve know of programmers who doesn’t even know how to do a “unsafe cast”, which has a benefit of throwing a early InvalidCastException, whereas whe using operator “as”, if you don’t check for nulls, will throw a NullReferenceException. I think the latter is a worse mistake — I prefer exceptions that describe the problem.

    I discussed this in an article here (http://blog.brunobrant.net/2011/08/careful-with-c-as-operator.html).

  • Tanya

    I would like to add some more common mistakes,

    1. Using. ToString() instead Convert.String()

    We should not use.ToString() blindly, since it will throw null exception, where Convert.ToString() will not throw the exception instead i will return empty string.

    2. LinQ- Use Count() instead of Any()

    Count will loop through all the items available in the collection whereas Any() will check items in the collection, if it finds first element then it will return.

    eg: Incorrect

    If(Coll.Count()>0)

    {

    // Statement

    }

    Correct

    If(Coll.Any())

    {

    // Statement

    }

  • Pingback: 8 Most common mistakes C# developers make | Het leren van SharePoint ontwikkeling

  • albertindian2000

    Actually string builder is very efficient for a larger data set of strings that need to be concatenated. For a very definite set, StringBuilder is an overhead and we can use string.join method.

    I would prefer the below model to throwing exceptions (incase of Exception ex)

    try{
    }
    catch(Exception){
    throw;
    }

    recent development in JIT produces same IL code for both foreach and for loop. I prefer foreach since i don’t need to worry about the length and index out of range issues.

  • bogus

    No, but on the other hand, if my banking software couldn’t find the entity to which I was transferring money I wouldn’t want it to blindly press on with taking money out of my account anyway.

  • bogus

    And yet for some code it means something has gone quite wrong, and you should stop, if you have a null value. So why reduplicate the null check when First() will do it for you?

  • http://www.vemployee.us/dot-net-programmers.html Stuvert

    If we don’t do any mistakes, we can’t learn.Thank you Pawel for great list. I think every c# developer who read this article will never do above mistakes in future. So i wish these 8 mistakes will disappear from “common mistakes” category in future:-)

  • Pingback: For vs Foreach | ALL IN SIGHT

  • lightwave

    humm most of these points are matters of taste (in or for a) context
    so i dont believe this was a well put together post but live and learn… i.e. just because you understand something doesn’t mean you can verbally express your thoughts well , or even teach

    i want to point out something everyone seems to be dancing around
    about the stringbuilder class and that is its context of use

    the best reason to use stringbuilder is because you dont want rampant garbage collection , you know strings are immutable … so you know what that means for … garbage collection right…?

    example

    we have 2 contexts
    1)we are building a string just once or very infrequently
    2) we are building/rebuilding a string in say a update situation or simulation looping situation

    in the case of 1 it really matters little so readabilty is the best choice

    in the case of 2 we have a serious matter of GC consideration
    which takes priority over readability in this case stringbuilder should be used as it can be used to avoid rampant Garbage Collection
    this is one place were regular garbage collection can be a problem in this context but in other contexts it is negligible

    this
    changes the nature of the mistake simply to newer programmers have not been taught or may not understand context were they should be using string builder.

  • lightwave

    and 3 is not a mistake to not know about some 3rd party tool
    id rather they know about the adapter pattern
    and or the other patterns that apply to this situation

  • Niyo

    [Difference Between C# Cast Syntax and the AS Operators] link is not working anymore

  • Tim Nesham

    I see there are a few issues with the original list! lol However, don’t get discouraged, this is an awesome discussion for someone recently picking up C# from years of Java – like me!

  • http://www.tezwingfield.co.uk Tez Wingfield

    8. Very interesting would be nice to run a test against multiple singletons vs one large result set.

  • Ognyan Dimitrov

    Mr. Bejger did you check the discussion/comments on the article at code project about Foreach vs For?

  • tomharrisnet

    Automapper is good but I’m not sure it is a simple “mistake” to not use it. Automapper uses reflection to find properties, in some cases you might decide to do your mapping manually for performance.

  • Vellanki Ganesh

    nice tutorial…very good information….