share
Stack OverflowCommon programming mistakes for .NET developers to avoid?
[+488] [116] amazedsaint
[2008-12-19 12:14:35]
[ c# .net vb.net common-mistakes ]
[ http://stackoverflow.com/questions/380819] [DELETED]

What are some common mistakes made by .NET developers, and how can we avoid them?

For example, trying to open a file without checking whether or not it exists, or catching an error unnecessarily.

[+480] [2008-12-19 12:22:47] TheSoftwareJedi
throw ex;

Instead of

throw;

The first example will reset the stack trace to the point of the throw, whereas the latter will maintain the original stack. This is crucial for debugging.

throw ex;

Is never right when being used to re-throw an exception.


I hate when someone does that. Especially when I'm the one trying to locate the bug :( - Ruben Steins
+1 for this. I've been writing .NET code for years, and somehow never came across this. It explains a lot :blush: - ChrisA
(37) I believe that even better is: throw new SomeOtherException( "More useful information", ex ); - Mike Scott
@Mike Scott unfortunately in more cases that leads to a 10 layer deep complex arrangement of SomeOtherExcpetion("Useless or obvious information", ex[nested ex, ex, ex, ex, ex]); - TheSoftwareJedi
2Jedi. 10 layers probably indicates too much exception handling. I've been using "Throw newEx(message,ex)" since forever, and never have >1 inner one. Also, Throw can only be used inside the Catch block, not in a called private method, so it's hard to abstract out the tidyup after the first Catch. - ChrisA
(11) @TheSoftwareJedi - lots of MS and 3rd party libraries do InnerException wrapping, it makes a lot of sense. So you just have to accept it as reality. The flaw lies with the debugger UI for exceptions - it should list all the exception objects conveniently instead of making you dig through the list. - Daniel Earwicker
(1) I wonder how many people are aware that you can stop in the debugger when an exception is first thrown, or that you can walk the chain of InnerExceptions from the debugger UI (it's just not that obvious). - Daniel Earwicker
(1) Yes, wrapping it up into a another exception is good practice - as long as you don't overuse this by putting it in every method 10 layers deep through your framework. - TheSoftwareJedi
(18) Everything You Wanted To Know About Exception Handling but were Afraid to Ask: blogs.msdn.com/cbrumme/archive/2003/10/01/51524.aspx - Mike Scott
@Mike Scott - Exactly. Here's the section from that long ass link that says what I'm trying to: "Between the 1st and 3rd forms, I suppose it depends on whether the intermediary can add important information by wrapping the original exception in a MyExcep instance" - TheSoftwareJedi
throw ex can be usefull, if you check you parameters by an underlying function for example. While dealing with interop'd dlls you often only get an bool as result and my require an errorcode, that needs to be translated... then a throw ex is useful. - BeowulfOF
@BeowulfOF correct, I was referring to the context where it is being RE-thrown. updated answer with that info. Thanks! - TheSoftwareJedi
Never experienced it, but would hate to! - Peter Morris
When your catch block is just outside of a call to a library that you do not have control of, e.g. a persistence framework, there does not seem to me to be much advantage in knowing the gnarly details of what went wrong inside it, as long as you retain the message they meant you to see (e.g. 'Error: UnexpectedRowCountUnclassified: The number of returned rows 0 did not match the expected count of 1.'). - Nij
(3) @Nij NO, NO, NO! :) if the try block contained multiple places where that error could have occurred, you wouldn't know which line it happened on. It NEVER hurts to maintain the original stack! - TheSoftwareJedi
@TheSoftwareJedi: I have ex.ToString() output in front of me that shows the line number inside the try block?! I'll still take 'my' way over what my colleagues do: catch (Exception) {}. That is to say, catch everything and don't handle it. I know there may be flaws in my approach (I know you are confident my ways are flawed :) but at least I am pretty sure I have improved on what was (not) there before. I hope you agree! - Nij
(1) Ooops! Quite right. Here I go grovelling. The line number was for the throw not the line that caused the exception - but you knew that :) Perhaps I was fooling myself because this was a one-line try block. Hey - I'm learning :) - Nij
@ Nij: catch Exception ex, log, handle, etc, rethrow IF DESIRED using "throw;" NOT throw ex;". Cheers! - TheSoftwareJedi
It's still possible to loose the stack trace using throw; - see weblogs.asp.net/fmarguerie/archive/2008/01/02/… - Justin
I personally think this should come up as a complier warning as I see no good reason for it. - PeteT
Yes, I just fell for this trap and had to find this question to remember how to properly rethrow an exception. They should really make it a compiler warning to use throw exc inside of a catch block. It is ridiculously useless. - Earlz
Here's a great article that talks about the various kinds of exceptions, and how to treat them. "Let them bubble up to the top" is a good rule for some exceptions, but definitely not for all (e.g. file I/O operations): blogs.msdn.com/b/kcwalina/archive/2007/01/30/… - Kyralessa
@TheSoftwareJedi, I never knew this. Is there any equivalent problem/solution in Java? Though there we're always wrapping in RuntimeException :) - Yar
(2) Resharper warns you if you do this. - Carra
1
[+374] [2008-12-19 14:17:51] John Sonmez

I always get hung up on this one.

string s = "Take this out";
s.Replace("this", "that");  //wrong

oooops didn't actually change s....

s = s.Replace("this", "that");  //correct

Its pretty easy to make that mistake.


(18) Dude, that ALWAYS happens to me, but only with Replace! With SubString and the like, that never happens, but with Replace that ALWAYS happens to me! - BFree
(2) Yeah it's an easy mistake to make, String is immutable so remember that you can never modify the contents of a String, just reassign the value to another String. - weiran
(7) Really nice that it is this way though. - corymathews
(2) It would be nice if Visual Studio warned you about this type of mistake. - Uhall
(233) It would be nice if there were a "Code has no effect" warning for that sort of thing. - Kyralessa
(2) FxCop will catch this problem, warning you that you're ignoring the return value. - jalbert
Yeah, the compiler can't be your babysitter. But jalbert is right, this is just calling a function and not doing anything with it's return value .. tools like FxCop do the job. - BobbyShaftoe
(24) The compiler ought to error if you discard the return value of a function unless you prefix the call with (void). Valid reasons for ignoring return values are so unusual that it would have be worth the minor inconvenience. Too late now. - Daniel Earwicker
I find this one particularly confusing because String.Replace leaves String intact and return the modified result, whilst Array.Reverse works in-place and returns void... - Dylan Beattie
This one got me a couple of times, too. - Dmitri Nesteruk
(1) hah yeah that one gets us all at one time or another, worst thing is makes you feel so stupid for doing it, its good to know there are lots of developers out there as dumb as me! - Agile Noob
i do this at least once a week, lol - Neil N
i used to get stung by this one... lol - GordonB
I did this just today actually! But with Remove() and Insert() instead! ;P - MiffTheFox
(27) It makes sense as long as your remember that strings are immutable. - AgeKay
I was doing this just the other day. I like the way it is though, having to reset the variable to keep the same identifier. - Chet
(1) Makes me long for gsub and gsub! in Ruby. The former does like C#'s Replace, not modifying the calling string. The latter, with the exclamation mark, is destructive: it modifies the string that calls it. - Sarah Vessels
I always make this mistake too - glad I'm not alone! Wonder if you could write an extension method that actually worked like this, though? - Dan Diplo
I did it like 100 times LOL - martani_net
I always found the "immutable" concept confusing, because if you do s=s.method() then you HAVE changed the original string. - DisgruntledGoat
(14) @DisgruntledGoat: No. You've given s a new referent; the original referent is unchanged. That's immutability. - Jason
(1) This happened to me all the time when I first learned C#! - Dinah
i've done it too, many a time. But ever since i really pondered what immutability meant for an object, i haven't made the same mistake. Why is that so important? Because when you really think about what immutable means (en.wikipedia.org/wiki/Immutable_object) it really sinks in. Later you see a string object and you call one of its methods that is supposed change the object's state, your "immutability" sensitivity should fire and tell you: "Hey! There's gonna be a string returned from this method that you need to use! Ain't nothin' gonna happen to this string at all!" - Paul Sasik
(3) Sometimes discarding the result of a function call makes sense. For example, List.Remove(..) and StringBuilder.Append(). Very often, we don't care about these results. What we need is a way to distinguish between functions where it does and does not make sense to ignore the result. If we can't change the syntax of C#, we could add an attribute to the function. Also, if the compiler could see that a class was immutable (e.g. with another attribute), it could issue a warning when we ignore results, although there may be false positives, like WriteValuesToConsole(). - Neil Whitaker
(4) I also think there should be some kind of standard (maybe possible in naming conventions only) regarding methods on immutable vs mutable objects. For example this method could be called s = s.GetReplaced("monster", "zombie") or (as such methods are chainable) s = s.WithReplaced("monster", "zombie").WithReplaced("chainsaw", "pump gun"). Also immutable collections should have an API that is different from mutable ones and adhere to some standard e.g. myFavoriteMovies = myFavoriteMovies.WithAdded("Night of the Living Dead").WithAdded("Friday the 13th").WithRemoved("Titanic"); - herzmeister
(2) Everyone forgets that strings are immutable. - fastcodejava
@Earwicker: There are valid reasons for disregarding the return value from a method, sometimes. Better to emit a warning than an error. - Chris Charabaruk
Thanks, I didn't knew that. - iSid
(4) Thankfully this is one of those mistakes that almost always makes itself apparent as soon as you test or debug it. Maybe if C# had [System.Diagnostics.Contracts.Pure] built in from the beginning and used all over the base class libraries, the compiler might have been able to give a warning whenever a pure function's return value is discarded. That would have been pretty damn cool. - Rei Miyasaka
Saying "this is OK if you remember strings are immutable" is just saying "this is one example of many gotchas that people find hard to remember" in bigger words. I do think the .Net framework makes the String object unecessarily hard to use. In fact strings were immutable in VB6 too, but that very rarely led to programming errors. The equivalent VB6 is a simple function call s = Replace(s, "this", "that"). In fact strings are immutable, so after this statement s points to a new string. But that's completely obvious from the API design. - MarkJ
That is one of the biggest and silliest mistake i did - Vamsi Krishna
I always do this with list.Concat( list2 ) - Patrick Scott
2
[+320] [2008-12-19 14:32:18] Pwninstein

Don't use "magic numbers" in your code.

Ex:

if(mode == 3) { ... }
else if(mode == 4) { ... }

Use Enumerations wherever possible, so the meaning, not the number, is exposed:

if(mode == MyEnum.ShowAllUsers) { ... }
else if(mode == MyEnum.ShowOnlyActiveUsers) { ... }

(I could have used a switch statement here, as well)


(31) That's always a good idea regardless of the language that you're using. If not enumerations, at least use named constants. (The is actually one of the rules in the coding standards of the company that I work for.) - RobH
(3) This also allows you to define your constants in a central place and make only one change rather than hunting all over the code for the changes that you'd have to make otherwise. - RobH
Right, regardless of language this is definitely a "best practice." - BobbyShaftoe
(3) Same goes for magic strings. Don't put the same string literal hundreds of times in your code. What if you spell it wrong somewhere? - Daniel Earwicker
Is it possible to have enumerations vailable throughout the whole project?? - Kiewic
If I have 'if' over an enum, I'm probably better off with 'switch'. - Jay Bazuzi
(3) While I agree with this as a best practice, I think pragmatism is appropriate here. Taking this practice to the extreme will leave you with dozens of constants that are only used in a single place in your entire application and are a nightmare to sort through. - rmz
Enumerations are great... And to answer Kiewic, yes you can.. in a previous windows app i've used a public enum throughout the project.... - GordonB
+1 Because this is something that should be followed regardless of language - byte
That's an old one... - ilya n.
(4) IMO you should put the const first: if (CONST==value), so that you will get a compiler error if you typoed == to be = instead. Goes for any language. - Commander Keen
(28) @Commander Keen: In C#, implicit conversions from int to bool are forbidden. - Jason
I've never thought to use that before, good tip! - Jamie Keeling
@Commander: Not for any language. VB.Net use = for both affectation and equality checks. - Clément
@Clément: Then Commander's advice applies even more strongly to VB.Net. You are right that not all languages perpetuate the absurdity of making assignment and equality look similar. F# uses = for equality and := or <- for assignment. - Joh
(2) @Joh: It doesn't apply to VB.Net at all. You cannot assign in the body of an If statement in VB.Net. So writing If Const = Value or If Value = Const is exactly the same, and will never produce any error. If you write A = B = C in VB.net, then the boolean A will store true iff B equals C: the meaning of = is contextual. The bug that @Commander mentions just does not exist in VB.Net. - Clément
Regarding F#: there wouldn't have been a risk in OCaml (or F#), since anyway the signature of the assignment (:=) is 'a ref -> 'a -> 'a ref, while that of the comparison is 'a -> 'a -> bool. So you could pretty much use = and == without problems. - Clément
(1) @Commander Keen: It's called yoda conditions. Hard to read, it is quite. stackoverflow.com/questions/2349378/… - Keyo
(1) I rather call these magic values since they can be anything... strings, numbers, characters, dates, regular expressions... - Robert Koritnik
@Robert - Good call, you're right! - Pwninstein
Totally agree with @rmz. Do it if it's used in more than one place. Otherwise, don't waste time creating an unnecessary type or constants. Move on and continue writing the rest of your app. - dbarros
3
[+255] [2008-12-19 23:32:29] Patrik Hägne

Oh, I forgot my number one pet peeve, over specification of input parameters. Let's say we have a method that takes a collection of some type, always allow the least specific type of collection needed by the method.

This is what you see A LOT:

public void Foo(List<Bar> bars) 
{
  foreach(var b in bars)
  {
    // do something with the bar...
  }
}

As you see the method does nothing but loops through the list, so requiring a list is an over specification, all you need is something you can loop through, ie. an IEnumerable<Bar>.

This is a lot nicer to the caller:

public void Foo(IEnumerable<Bar> bars) ...

If the method requires to know the count of the bars collection use ICollection<Bar> instead, or maybe you need to access bars by index, then use IList<Bar> but always the least specific sub type needed.


(2) Yeah, this is a good tip. You don't see this tip very often. Most of the others in this list are good but sort of cliche, this is more original. :) - BobbyShaftoe
(62) On the other hand, methods that take interfaces can be a little harder to figure out how to use. Example is TextRenderer.DrawText, which takes an IDeviceContext as a parameter. Took me a bit to realize that a Graphics object implements IDeviceContext. - MusiGenesis
You're right that in some cases you trade off the learnability for flexibility, but that must be edge cases. I still think this is most common for collections and there should be no problem for anyone to understand what IEnumerable(T) means. - Patrik Hägne
(38) Resharper tells you to do this. - Daniel Earwicker
why not ask for a simple array? - Andrew Harry
(15) Ohh, Harry, if you every do that people will hate you. Actually don't ever use arrays for anything else than local variables (OK, there might be some other cases but that's a good rule of thumb). In this case it's just not needed, use the interface and you can pass an array, a list or something else - Patrik Hägne
(6) Agreed in general, MusiGenesis, but not for IEnumerable(T). Essentially every generic collection implements it (even stuff like Stack(T) or Queue(T)), so there's no reason to use anything more derived. - Kyralessa
(1) public void Foo<T>(IEnumerable<T> bars) where T : Bar ... is nicer still. - finnw
(5) @finnw, I don't agree, mostly because the type constraint is not part of the signature so you could not have an overload for "foos". I see your point in .net 3.5 and earlier but in .net 4.0 there's absolutely no point in doing this since you have co- and contravariance. - Patrik Hägne
(2) F# would have inferred the most general type. - Jon Harrop
(3) Take String.Join for example – before .NET 4.0, it needed to be called by passing an array: String.Join(string delimeter, string[] strings). Now, it is possible to hand over an IEnumerable<string> which is perfectly acceptable because the method does not have to restrict the specified collection type – it can act on it as long as it is enumerable. - Marius Schulz
I should have read this answer long long ago! - Mr.TAMER
4
[+229] [2008-12-19 12:19:13] Marc Gravell
[1] http://stackoverflow.com/questions/371328/why-is-it-important-to-override-gethashcode-when-equals-method-is-overriden-in-c#371348
[2] http://stackoverflow.com/questions/379041/what-is-the-best-practice-for-using-public-fields
[3] http://stackoverflow.com/questions/365615/in-c-which-loop-runs-faster-for-or-foreach#365627
[4] http://stackoverflow.com/questions/254969/dealing-with-net-idisposable-objects#254971

This is great, and interesting how many of those represent language gaps. Structs could have been disallowed except in unsafe blocks (for interop), Equals/GetHashCode could have been part of an interface, non-private/non-internal fields could have been banned, we need better IDisposable support... - Daniel Earwicker
(13) Structs do serve a useful purpose - but the terminology often makes people think of C structs, which are sooooo different. - Marc Gravell
(6) Why do you think structs should be immutable? Aren't they often used as optimizations so you can pass small bits of data around on the stack instead of bothering the heap. Restricting structs to being immutable possibly means you're denying some further optimizations that could be made? - Scott Langham
Preferring immutability is a good guideline for reducing complexity because you can rely on immutable objects to not change their state. But.. is there any reason why designing immutable structs is somehow more strongly preferred to designing immutable classes? - Scott Langham
I found that reSharper lets you refactor between for and foreach - very useful for post-hoc optimization. - Dmitri Nesteruk
I did the Static-class-in-a-web-app error once. Never again! :-) - Rune Grimstad
What are the problems with struct? - dr. evil
(6) @fm - the biggest problem is people making "mutable" structs, without realising the absolute chaos this can quickly cause. Oversized structs can put stress on the stack (and CPU, as it has to blit them often). Boxing is a minor pain point, but not as big as you'd imagine. But in most cases, people are thinking of them as "objects without methods", which is false on two different levels (they aren't "objects" as such, and can have methods). I commonly see people type struct Person etc; it should be a class. struct should be reserved for values only. - Marc Gravell
(7) The only problem with static in a webapp is most web devs don't think in terms of web safety... this can also affect instances of static classes in libraries in web apps that aren't threadsafe. Saw about 4 major weirdness issues related to services w/ unsafe clients used statically. You can cut yourself with a knife, but its hard to be a chef without one. - Tracker1
@Tracker1 - I like the analogy. I think we agree inappropriate use is the problem. That is true of most things, of course - but with web the balance of inappropriate-to-appropriate (for static) is so heavily shifted. - Marc Gravell
What's wrong about static in web apps?? I use it all the time in http modules, http handlers, http applications together with a ReadWriterLock from System.Threading. It works great...??? - Henrik
(4) If you synchronize (via lock, ReadetWriterLock etc), then it will at least work... but many developers use static without any locking (and it works fine on their machine)... "scales to 1" ;-p But even with locking you need to be careful about the amount of blocking - if the locks are too long, you can cause big problems. But used correctly static can be fine... but it is rarely used correctly. - Marc Gravell
I agree with the struct advice. Structs have value type semantics, which can be confusing when you're expecting reference type semantics. You can pass an object reference to a method and access the object directly, but if you pass a struct, you're copying the struct and all of its fields, and any modifications within the method will only affect the in-method copy. This can easily lead to confusing bugs if you're not expecting it. - Kyralessa
I had a problem with a static variable in a web app once. The data in the variable suddenly being shared across sessions caused some headaches that took a little debugging to see what wasn't working as I thought it would work... - JB King
struct are usefull when the equality of an object is determined by its data. (Address for example) - Nicolas Dorier
True, but classes can have equality operators (etc) too... - Marc Gravell
(1) I agree that structs should be immutable. Structs do have one unique feature that classes cannot offer: they are never null. They also have some small perks, like getting value equality without writing any code and better performance if they are very small. But I agree that a class is better in most cases. - Neil Whitaker
int? is a struct, and can be null (depending on how you define null) ;-p - Marc Gravell
(1) int? is not a struct, int? is the same as INullable<int> which is an object. - Sekhat
(4) @Sekhat - no, it really isn't. int? is defined as Nullable<int>; Nullable<T> is a generic struct (not an object). There is no such thing as INullable<int>, and even if there were if would be an interface, not an object. Even if it was - even though interfaces are generally treated as ref-types, there are some edge-cases where they are non-boxing. Note, however, that even though Nullable<T> is a struct it is excluded from the : struct generic condition, and has special compiler and runtime treatment for null etc. Bit it still remains a value-type / struct. - Marc Gravell
I was working off memory and meant Nullable<int> :P I didn't know it was a generic struct. :) Nor the rest of it. Well thanks for the info :) - Sekhat
5
[+220] [2008-12-19 12:24:24] Greg Dean

Not using a using statement to ensure that Dispose() is called.


Hah! Try missing a Dispose() on a mobile device and see how long your app runs before it pukes. Been there, missed that. ;) - Mat Nadrofsky
(23) This is very important for Dialogs as they are not normally disposed of by the garbage collector. - Chris Porter
(2) I'm not sure how preferring "using" over calling "dispose" is necessarily much better or will necessarily solve the problem. Don't get me wrong "using" is good practice.. but if you are going to forget to dispose of something, aren't you likely to forget the "using" block anyway? - BobbyShaftoe
(11) @BobbyShaftoe - think about the effect of exceptions. You should at least use try/finally to ensure that Dispose is definitely called. Using statements are a more convenient form of that patter. - Daniel Earwicker
(3) The trick is to put a Debug.Fail in a conditionally compiled (#if DEBUG) finalizer, and GC.SupressPendingFinalizers() in Dispose. Then you'll catch it at runtime, in DEBUG. - Jay Bazuzi
@Jay Sounds like the real tricky part is being able to control the source for all of the IDisposable type that you use, or you subclass them all. I suspect most people do not have this luxury. - Greg Dean
I wish they would modify the syntax to allow one using to dispose multiple objects. I often get SQL statements disposing of the connection, command and dataadapter causing lots of nesting. - PeteT
@BobbyShaftoe: Calling StreamReader.Close() can easily be forgotten or accidentally be deleted whereas it is hard to accidentally remove the using() block's curly braces. - Marius Schulz
I made this mistake when loading and processing images and I got a lot of GDI+ generic Exceptions :( - yelinna
6
[+186] [2008-12-19 13:12:05] Scott Langham

Use this cast:

Tree tree = obj as Tree;

... only if the program logic is such that you anticipate obj may or may not be of type tree.

In the situation where you expect that obj will only ever be of type Tree, prefer this style of cast:

Tree tree = (Tree)obj;

Prefer a (TargetType) style cast unless you really do need to make use of the conditional functionality offered by an 'as' cast.


Note: be sure to follow an 'as' cast with an 'if' or other appropriate logic to ensure that if the result of the 'as' was null, an attempt won't be made to dereference it. This is a mistake:

Tree tree = obj as Tree;
tree.GrowBranch();   // Bad. Possible NullReference exception!

In this case, the programmer meant one of these:

// Expected obj always to be a tree
Tree tree = (Tree)obj;
tree.GrowBranch();

// Expected obj could be a tree or could be something else
Tree tree = obj as Tree;
if( tree != null )
{
    tree.GrowBranch();
}

Some people believe that...

Tree tree = (Tree)obj;

...is bad because it may throw an exception if the prerequisite that obj is a Tree isn't met. It's not bad though, because it will throw an InvalidCast exception. That's the right sort of exception and is thrown at the right time.

The NullReference exception that occurred after the 'as' cast in the first GrowTree() example gets thrown:

  • When the real cause of the problem was not a null reference, it was an invalid cast.
  • Some time after the real problem (the bad cast) occurred.

These two reasons make it more difficult to debug and determine what the real problem was.

The performance of these two types of cast is similar. It is true that a (TargetType) style cast throws an exception if the cast fails. However, this is not a problem that would affect performance. The reason is that we use a (TargetType) style cast only when we expect the cast will always succeed. So, no exception should ever be thrown! If an exception does get thrown, then there is a problem in the logic/design of the code. Fixing a problem like this by changing the (TargetType) cast into an 'as' style cast is probably wrong as it will probably just mask the real cause of the problem.

Using the 'as' cast instead of the (TargetType) cast because you think it looks prettier is not a good reason for writing incorrect code.

Writing:

Tree tree = obj as Tree;
if( tree != null )
{
    tree.GrowBranch();
}

every time you need a cast, "just to be on the safe side" is absurd. You have to stop somewhere, otherwise one day you'll find yourself writing:

if( thisComputersPowerHasFailed )
{
    SendEmailToAdministratorToSaySomethingHasGoneWrong();
}

Code like this introduces more and more conditional execution paths through your code. Every time you write some code to cope with a case that you don't expect should happen, you will increase the complexity of your program. Unnecessary complexity is just the kind of thing that causes bugs to slip in to code. The root causes of bugs will be tricky to find because they'll be hidden behind other unnecessary error handlers that try to hide or log the problem and carry on. A (TargetType) cast adheres to the generally good advice of writing code to fail-fast.


(13) totally agree. the direct cast shows a programmer assertion and gives a more understandable exception on error. - TheSoftwareJedi
(1) -1, cause obj as Tree is much faster, even including the necessary if(obj != null). On failing this is faster although since exception raising costs really much time. - BeowulfOF
(1) -1, I agree with BeowulfOF, performance wise it makes more since to do the "as" because you can always test for the null and not have to worry about it blowing up and than having it throw an exception. - Donny V.
(22) BeowulfOF - this is a myth. "obj as Tree" is not much faster if the cast succeeds (only if the cast fails and you save throwing an exception). - Joe
(1) Agree with Jedi. If its gonna fail anyway, make it fail as early as possible, with the most relevant exception/message... - Arjan Einbu
(3) I agree with Scott here +1 .. When you do a as you always risk passing around a null object that may rise a NullPointer later on (masking where that null came from). Use "as" if you expect nulls as ok input, but if the null is an exception to you, let it throw one! - Tigraine
(2) Exceptions are for exceptional cases only, not program flow. - John Sonmez
John Sonmez, exactly. The "as" keyword isn't just there to make Anders feel better about himself. - BobbyShaftoe
(1) Many methods in the BCL have a Try version. "as" is like "TryCast". If you know the cast may not work, use 'as'. If you'd regard a failed cast as indicating a bug in your code, do a normal throwing cast. - Daniel Earwicker
@Jedi, Tigraine: Exactly... you've hit the nail on the head, the exception will be for a failed cast, instead of for a null exception occurring sometime later that is tricker to track down. - Scott Langham
Note.. I wasn't saying never use 'as'. 'As' is perfect if you anticipate obj might not be a Tree... just remember the appropriate 'if' afterwards so that code can be written to deal correctly with the two possible conditions (obj being a Tree and obj not being a Tree). - Scott Langham
(1) It's unfortunate that the C# language designers decided to use the C-style syntax for casting. It is ugly. - Jay Bazuzi
(1) You should use the 'as' variant. Don't C-style cast. - sixlettervariables
(2) I disagree - C style cast is generally a bad idea. A common source of bugs is when programmers cast to an expected type and an exception is thrown when they are wrong, crashing the program. Much better to use "as" and a check for null - as well as focussing the programmer on handling the wrong-type case properly, it's more efficient than using a try/catch around your cast. - Jason Williams
(2) @Jason. I didn't say anything about putting a try/catch around the cast... although I'd expect you to have at least hooked the UnhandledException event for you application so you can log the failure. The two types of casts perform about the same. Try/catch isn't slow.. but throwing an exception is. And one shouldn't be thrown if you're using a non-as cast, if you're expecting an exception... use the 'as' cast. - Scott Langham
(2) @Jason. Also. I can't imagine how you'd handle this error. If a non-as cast fails, then there's a problem with the application logic. All you can do is log the exception and then terminate the application. It would be a mistake to let the application continue because it would be running in a non-deterministic state, and there's no knowing what damage it could cause. One thing you could do in a handler is throw an exception, but then why would you bother writing this handling logic when you could have written a non-as cast that would have done that for you. - Scott Langham
(2) -1. You need to consider the situations that you're casting in. You will not always want to throw an exception when a 'cast' doesn't work. Effective C# goes into this in some detail. The way to use 'as' here is to use the fact you can examine tree to see if it is null or not: Tree tree = obj as Tree; if (tree != null) {tree.GrowBranch();} else {//whatever}. This gives you more control than relying on catching exceptions. - Martin Clarke
(4) @Martin. I agree with you entirely. In the answer, I said an 'as' cast is perfectly acceptable for the type of situation you've described. I did not say never use the 'as' cast, which is what a lot of people seem to think is what I said. Maybe I'll edit the answer to make that clearer. - Scott Langham
(3) Thoroughly agree with this answer. Strongly disagree with those suggesting using the 'as' operator. To reword the posting "Don't use as for conditional casts where the type conversion is part of the function's contract". In the above example part of the function's contract is that 'obj' if of type Tree. You should not be writing additional code to attempt to 'avoid' broken invariants. However, what you sometimes do want to do is also add explicit precondition checks - Phil
(1) Thank you for the thorough explanation for your recommendation. As a newbie, I appreciate hearing the reasoning behind it. - PRINCESS FLUFF
(2) I can't for the life of me figure out the reasoning that goes on in the heads of the downvoters here. Using 'as' plus an if-check is actually equivalent to saying "it's okay for this cast not to work". But generally, the cast is supposed to work and if it doesn't, there's a bug. To say that, use the C-style cast. That's what it's there for. I'd hate to work in their code where invalid behavior is swept under the rug to avoid an exception. - siride
(1) This is the worst; I have a colleague who does it regularly: Tree tree = obj as Tree; if ( tree == null ) { throw new InvalidCastException(); } The only defense I can imagine is to let the "as" cast fanatics know that you really do want an InvalidCastException there if the object is not a Tree. If you write Tree tree = (Tree)obj; they might (in their ignorance) just think you're ignorant, and change it to something worse. - phoog
An as with an if statement is far better than a try/catch as far as performance goes. However if you want to ensure the cast works the way you want do the C style cast. You will ensure you only get a valid cast that way. I personally never use C style casting I tend to use the as since it's faster and catching InvalidCastOperation or whatever is seems like using try/catch for program flow which I see as bad. - Justin
(1) @Justin and what do you do if obj must be a Tree? Do you just return from the method and hope nothing goes wrong? Or do you throw a different exception to signal something went wrong? And you should never ever even want to catch an ICE. If I ever see a colleague do one of those things, I'll do something very mean to him. - Patrick Huizinga
This will make me think twice before using as. I typically use it in UI event handlers, like casting the sender. But, I'm sure I've used it in cases where if I cast an item it should be only one thing. - townsean
The is syntax can also be used in place of a null comparison: if (obj is Tree) { Tree tree = obj as Tree; } although there is likely some additional overhead here, the code is quite clean. - dodgy_coder
7
[+166] [2008-12-19 13:13:22] Scott Langham

Not unhooking event handlers appropriately after wiring them.

This is because each event registration causes a reference to be created from the event source to the object/delegate that handles the event. These references can easily form part of a path of references from a root heap object to the handler.

Any object that has a path of references from the root heap object will not be garbage collected.


I was typing the same thing when you posted yours! - TheAgent
(6) In my opinion this is one of the biggest things - people just assume the magic garbage collector will take care of things. If you understand the event system in .NET, you know painfully well that that is not true. - unforgiven3
Circular references are garbage collected. I assume you meant hidden live references which a lot of developers don't realize is what you get when you hook up an event handler. - Stephen Martin
Stephen is correct - circular references are GCed properly. - RoadWarrior
Actual problem is very simple - if you want your object to be collected, don't add it to some list that will live for the entire process lifetime. Registering with a static event (or an event on your main window) is an example of this. - Daniel Earwicker
@Stephen: Thanks for the correction. I've corrected the answer. - Scott Langham
(8) +1 because I got stung by this. - geofftnz
(10) Don't forget: (Apart from forms objects) your dead object will still be called when the event is raised, causing all kinds of bizarre effects and leaving programmers wondering why they "seem to have two of everything even though there is only one instance of their class". - Jason Williams
Oh yeah this is a big one. I sort of wish weak events were the default, though that has its own problems too. - Rei Miyasaka
It's so frustrating that I can only upvote you once.. - Andrei Rinea
I've read about this many many times, but I've never read a good low-level explanation of why this happens. Any recommendations? Thanks. - phoog
8
[+159] [2008-12-19 12:19:17] Treb

The most common error I make is starting to code without thinking first. I still catch myself doing it from time to time...

Does happen when I work outside the .NET framework, too.

Another bad habit (which I successfully dropped) is swallowing exceptions:

try
{
    //Something
}
catch
{
    //Do nothing.
}

Understanding the pitfalls of exception handling took me some effort, but it was worth the time I spent on it.


(2) +1. I wish more developers would take more time to truly understand exception handling. It is one of the most misunderstood and abused features in the whole .NET Framework. - joseph.ferris
(4) Oh yes, I have done my share of abusing it... - Treb
What is a recommended authority on .NET exception best practice? - Jimmy
don't use them unless you have to. They're like beer, good in moderation, but you can give yourself one nasty hangover if you have too many. - gbjbaanb
(10) I'd appreciate if you elaborate on this one: I use it regularly and would love to know if I must get rid of it. Eg.: try { File.Delete(blah); } catch {} to avoid reporting an unlikely file deletion problem that user will not understand anyway. - Serge - appTranslator
(2) @Serge: There are a few conditions where its ok to do so. In general its better to at least log the exception information, so if the program crashes you know where to look for the cause. See stackoverflow.com/questions/313839/… for example. - Treb
(12) I have been lobbying the management to make swallowing an exception without even a comment on why it's being swallowed a fireable offense. No luck yet. - James Schek
(3) We had a guy that was upset that we would catch exceptions log them (via email or DB call) and then return an error from some of our libs, he wanted us to catch the exception, and then re throw it so another try catch could catch it vs. just checking what is returned - Bob The Janitor
+1 Had to waste lots of time fixing apps with random catch-alls. And the data inconsistencies they leave behind. - Andomar
(19) @Bob The Janitor - I agree with that "guy". He is correct. Why swallow the exception in your libs? - Bobby Cannon
(1) @Bobby Cannon: For a long time I was doing exactly that: Catching all exceptions and returning false in the catch block. This is what happens when you learn C and get to know exception handling by the way of 'there are soem new keywords you can use: try, catch and finally'. - Treb
This is Java-ish - SztupY
Occasionally I get forced to put in a catch with nothing happening. It always gives me a bad feeling. A recent example was displaying a tooltip on a grid, occasionally for some reason it would go out of scope and not get the correct row handle so I needed a cacth just to stop it crashing. - PeteT
Once I worked with a programmer that "swallowed" any logging exception in the code - horrible... - Eran Betzalel
(7) Pokemon exceptions - when you just gotta catch 'em all. - vash47
9
[+133] [2008-12-19 13:07:37] Cory Foy

Deploying your ASP.NET applications to production with Debug="true" set in the web.config. The compiler can't do any optimizations, and batch build is disabled. When we used to debug performance problems, this was one main area we'd look at. Tess has a great article [1] on it.

It's so common, there is a built-in command to the SOS extension to WinDBG. Just get a memory dump of an ASP.NET application and run:

!finddebugtrue

which would output something like:

0:016> !finddebugtrue

Debug set to true for Runtime: 61b48dc, AppDomain: /MyDebugApplication

Debug set to true for Runtime: 1f50e6d8, AppDomain: /MemoryIssues

Total 16 HttpRuntime objects

Tess' article above has more examples.

[1] http://blogs.msdn.com/tess/archive/2006/04/13/575364.aspx

(1) +1 because I do that too often... - Andy May
While I generally agree with that, I think that having a detailed stack trace when an exception occurs (namely, the line number) is very valuable, therefore I usually leave debug="true". - Dario Solera
I've done that a couple of times.... i'm going to have a look to see what sort of performance impact is has now. - GordonB
(5) Dario - You can still output PDBs even with debug turned off. For any production application, you really should never deploy with Debug=true. There are much better ways to troubleshoot your apps without taking the performance hit. - Cory Foy
Can you get the line numbers if it's a Web Site with debug="false" as opposed to a Web Application? - Min
(36) System admins should really add this to machine.config on production/live servers: <System.Web> <deployment retail="true" /> </System.Web> This over-rides any debug="true" settings in web.config - Dan Diplo
@Dan Diplo thanks for the tip I have been doing it manually. - PeteT
10
[+119] [2008-12-19 15:07:36] DaveK

If you are going to be doing a large amount of string concatenation, use the System.Text.StringBuilder object.

Bad:

string s = "This ";
s += "is ";
s += "not ";
s += "the ";
s += "best ";
s += "way.";

Good:

StringBuilder sb = new StringBuilder();
sb.Append("This ");
sb.Append("is ");
sb.Append("much ");
sb.Append("better. ");

(30) In this example I'd rather do s = "This " + "is " + "not " + "the " + "best " + "way."; stackoverflow.com/questions/21078/… - Greg
why is stringbuilder better? learning...thanks. - johnny
Johnny, each time you append to a string, it recreates the value in a new memory space. Using StringBuilder gets around this by handling in a more dynamic manner. A quick search should give you plenty of references to the specifics. - Chris Porter
shh .. somewhere Jon Skeet is cringing ... The most common post I see from him is how the "StringBuilder is preferred of concatenation" is complete nonsense. - BobbyShaftoe
(59) I disagree with blanket use of StringBuilder. If you're only doing 5 concatenations in the above example then StringBuilder might actually be slower (due to the overhead of constructing it). But any difference will be negligible either way. It only really matters for thousands of concatenations. - EMP
(18) String has a Concat method that can accept multiple strings. If you write s1+s2+s3 the compiler is smart enough to turn it into a single Concat call. However, if you're calling s1+=s2 in a loop that may get long, use StringBuilder (sb.Append(s)) instead. - Daniel Earwicker
(15) I wrote a simple test app on this, and under 4 concatenations string.concat is fastet. From 4 concatenations up, stringbuilder is faster. Try it, write it, use System.Diagnostic.Stopwatch. - BeowulfOF
Good advice, this is one of those little things that irks me as well :) - hmcclungiii
(21) Does this really matter? codinghorror.com/blog/archives/001218.html - Gary Willoughby
I'd only using stringbuilder if doing a lot of concatenation, in a loop or if i knew there was going to be a lot of manual concatenations.... Otherwise the first example does suit. - GordonB
Gary Willoughby: yes. Read Dennis Forbes's comment below the article. Using += is slow, but concatenating a bunch of strings in one expression is not. - Neil Whitaker
(1) String concatenation using + operator is a real performance killer when the size of the strings is somewhat large, concatenating strings that will yield a resulting string of a few hundred bytes won't make any perf. difference. - Pop Catalin
(1) I actually optimized an aspx page by replacing string concats with a single stringbuilder in an application. The page took a minute to load with concats. Several seconds afterwards. The aspx itself basically had a literal... - Min
You have to concatenate 1000's of strings before having a performance impact (and a huge one, granted, since you're in O(n^2) instead of O(n)). But the example is actually bad. You use StringBuilder if 1) You know you'll have 100's of strings to concatenate 2) you DON'T KNOW how many strings you will concatenate. - Yann Schwartz
I generally only use StringBuilder on loops (or incredibly long lists, such as over 50+ items). Anything else, I don't think there is a significant benefit -- from a performance stand point. However, from a consistency standpoint that is another matter. It's also important to remember that StringBuilder preallocates how much memory goes in to it and as such, if you know you're loops will be huge you can plan for this ahead of time and save yourself some milliseconds (which count in loops). - Nazadus
Please justify your answer - Avram
This is wrong... having the number of concatenations known makes the concat the better choice over the stringbuilder - Andrei Rinea
string.Format is really the answer here. For those situations where StringBuilder is too bulky, and string concatenation is not cutting it (it never is), - Mark Cassidy
As Jeff Atwood put it: It. Just. Doesn't. Matter! (see codinghorror.com/blog/2009/01/… for explanation) - Marius Schulz
(1) @Andrei Rinea: If you have a large number of concatenations, and you know how many there will be, then string.Concat(string[]) will be more efficient than StringBuilder. But repeated calls to string.Concat(string, string) will be far less efficient than StringBuilder. This is because string.Concat(string[]) adds up all the lengths of all the strings in the array, and uses that value for a single memory allocation. StringBuilder doubles its buffer whenever it fills up, requiring a few memory allocations, and string.Concat(string, string) allocates a new string every time you call it. - phoog
I actually went to the trouble of writing an app to test all the different ways I could think of to combine strings. Stringbuilder was rarely the fastest. Not using stringbuilder in some cases allowed the compiler to do the concatenations which obviously is much faster than stringbuilder. - dwidel
I use String.Concat - yelinna
(1) @Greg: In this example I'd rather do s = "This is not the best way."; :D - Nawaz
Worry about understandability first, performance later. If you are worried about performance, you should remember newing StringBuilder up with a capacity, if possible, so it dosent have to reallocate/resize its buffer. - FRoZeN
@Marius: I had a function that would take minutes and finally crash and burn handling files of 2+Mb, running out of the 4gb of RAM of my machine. Using stringbuilder, the same function runs in a fraction of a second on the same file sizes. So, IT DOES MATTER... if concatenation is in a long loop. - Sylverdrag
@Sylverdrag: Exactly that is stated in the blog entry I linked: "Every decent programmer knows that string concatenation, while fine in small doses, is deadly poison in loops" - Marius Schulz
11
[+112] [2009-02-09 17:29:23] Peter Morris

What I really hate is when programmers ignore compiler warnings and just leave them in the code.

If your project ends up with 100 compiler warnings that you consider "okay to live with" when the 101st compiler warning appears that you might not be happy with you are very unlikely to spot it, you're then likely to be introducing unexpected behaviour.

On a similar line, I also hate it when people change source code in a way that causes it to break unit tests and then don't fix the source code or the test so that they pass. I've been working on a solution that has had 9 broken test cases new for the past 3 weeks and it is driving me mad! Whenever I break a unit test it is harder for me to find what I have broken.


(14) If you're overrun with a particular warning that you don't plan to fix each instance of, you can disable it in the project settings. (Example: Warning 1591, undocumented public or protected method. Unless you're creating a reusable library to sell, you may not want to, nor have time to, document all those.) Go to the Build tab of your project properties, and in the Suppress warnings box, type the number of each warning you want to ignore, like this: 1591 1867 1883 This can help you reduce warning noise and see only the most essential warnings. - Kyralessa
(2) +100 (if I could) When does StackOverflow introduce multiple upvotes??? :) - gehho
(8) ... or as Scott Hanselman put it jokingly: "If they were important they'd be errors, wouldn't they?" - Marius Schulz
Scott Hanselman was wrong :) - Peter Morris
12
[+106] [2008-12-20 12:19:58] Daniel Earwicker

1. RAII (resource acquisition is initialization)

A stupid name for a great idea. In C++, constructors are mirrored by destructors. After some serious internal and external lobbying right before C# was released, MS added the using statement, providing at least minimal support for this idea, though there is more they could do [1]. But the usefulness of RAII is still not widely grasped. It's sort of true to say it cleans up "unmanaged resources", but think about what that means: anything other than memory. Think of all the places in your code where you modify state, and later want to put it back again.

Simple example - an Undo system. You want to support "batch" undo transactions, in which several actions get bound up into a single one. The application would do this:

undoSystem.BeginTransaction();

// do stuff, add several undo actions to undoSystem

undoSystem.EndTransaction().

The point is, EndTransaction MUST be called, however we exit the function, to restore the system to the state we found it in. You should at least use try/finally - but why not follow a pattern consistent with the language? Make BeginTransaction return an object:

public class UndoTransaction : IDisposable
{
    public void Dispose()
    {
        // equivalent to EndTransaction
    }
}

Now the application code can just do this:

using (undoSystem.BeginTransaction())
{
    // do stuff, add several undo actions to undoSystem
}

Now there is no need to correctly figure out which method is the "ender" for the "beginner" of the state (which in some situations would not be as obvious as in this example). And ask yourself - would it make much sense for UndoTransaction to have a finalizer as well? Absolutely NOT. Finalizers cannot safely call on to managed objects, and they run in a different thread. The one thing they are useful for (calling an interop API to dispose of a Win32 handle) is now done much more easily by using SafeHandle.

Unfortunately the internet and older books are riddled with advice about how IDisposable implies the need for a finalizer. Ignore them. And also not really explaining the implications of "unmanaged resources" - anything about the state of your program can be regarded as an "unmanaged resource". By taking advantage of IDisposable/using, you can apply a consistent coding style to deal with states that change in line with the method call stack. Which brings us to...

2. Exception Safety.

There are some operations that do not throw. Assignment cannot be redefined in C#, so:

x = y;

Assuming x and y are fields or variables of the same type, that will never, ever throw (except under truly bizarre circumstances where you can no longer rely on anything working). How reassuring is that?! But also useful. Think of how, often, one of your methods will update the state of the class it belongs to. Sometimes it will modify two or three (or more) private fields.

What if an exception is thrown at some point during this multiple-update of state? What state is your object left in? Will one of the fields be updated, but not the other two? And does the resulting state make any sense? (does it "satisfy the class invariant"?) Or will it later cause your code to get confused and cause further damage?

The solution is to figure out what the changes need to be, before doing anything to update your fields. Then when you have all the answers ready, do the assignments - safe in the knowledge that assignments never throw.

Again, because of GC, C# programmers have been encouraged to think that this is a C++-specific problem. It's true that exception safety (and RAII) are commonly spoken of in terms of deleting memory allocations, but that is just one example (it happens to be very important in C++). The truth is, exception safety is an issue that concerns any program that has non-trivial modifiable state in it, which is most programs.

Another issue with exceptions is that they are just as much a part of the "interface" exposed by a method as are the parameters and the return value. We are encouraged (by some of the people answering this question) to catch specific exceptions instead of just Exception itself:

try
{
    funkyObect.GetFunky();
}
catch (SocketException x)
{

}

How do you know that GetFunky throws SocketException? Either documentation, or trial and error. What if the author of that method later changes it so it doesn't use sockets, so it throws something else? Now you're catching the wrong thing. No warning from the compiler.

Compare with this cautionary tale:

IEnumerable<int> sequenceInts = funkyObject.GetInts();

// I found out in the debugger that it's really a list:
List<int> listInts = (List<int>)sequenceInts;

Very clever, until the author of GetInts changes it to use yield return instead of returning List<int>.

The moral is that you shouldn't rely on undocumented, untyped coincidences, you shouldn't sniff out the internals of a method you are calling. You should respect information hiding. But this applies to exceptions as well. If a method allows a huge variety of exceptions to leak out of it, then it has a very, very complicated interface, which its author probably didn't intend for you to be reliant on. It's not really any of your business how a method works internally.

This is all partly the fault of lazy library authors. When writing a nice clean modular library, consider defining your own exception type(s). Make sure that your library's methods ONLY throw your approprate exception types and document this fact. Your library methods' code will look like this:

try
{
    // do all kinds of weird stuff with sockets, databases, web services etc.
}
catch (Exception x) // but see note below
{
    throw new FunkyException("Something descriptive", x);
}

I call this normalizing the exceptions. Note that by passing x into the constructor of FunkyException, we cause it to become the InnerException. This preserves complete stack trace information for logging/debugging purposes. Also note that this contradicts the advice given by several other answers to this question (including the highest rated answer), and also many blog posts on this subject. But there it is; I think those people are dead wrong. Exceptions are part of the visible interface of a method, and it is just as important to control that aspect of the interface as it is to specify the type of the parameters and return values.

And when catching exceptions thrown by a badly written or badly documented method (one that may or may not throw all manner of exception types - who knows?) I would advise that you do NOT catch whatever specific exception types it throws, discovered by trial and error in the debugger. Instead, just catch Exception - wherever you need to in order to ensure the exception safety of your program's state. That way, you are not becoming dependent on undocumented or coincidental facts about the internals of other modules.

But...

Unfortunately catching (Exception x) is a really bad idea until CLR 4.0 comes along. Even then, it still won't be ideal, though not as bad as it is today. And yet, it has long been advised by the Exception Handling block of Microsoft's Enterprise Library!

For the details, see:

In short - if you catch all exceptions, you also catch fatal exceptions (ones that you want to cause your program to stop and capture a stack trace or a mini dump). If your program attempts to limp along after such an exception, it is now running in an unknown state and could do all kinds of damage.

Reponses to several comments from P Daddy:

"Your advice to ignore the conventions prescribed to by the majority of the industry, as well as Microsoft themselves..."

But I'm not advising that at all. The official advice on Dispose/finalizers used to be wrong but has since been corrected, so that now I'm in agreement with the majority opinion (but at the same time this demonstrates that majority opinion can be wrong at any given time). And the technique of wrapping exceptions is widely used by libraries from Microsoft and 3rd parties. The InnerException property was added for precisely this purpose - why else would it be there?

"IDisposable is not RAII... the using statement, as convenient as it is, is not meant as a generic scope guard..."

And yet it cannot help but be a generic scope guard. Destructors in C++ were not originally intended as a generic scope guard, but merely to allow cleanup of memory to be customised. The more general applicability of RAII was discovered later. Read up on how local instances with destructors are implemented in C++/CLI - they generate basically the same IL as a using statement. The two things are semantically identical. This is why there is a rich history of solid practise in C++ that is directly applicable to C#, which the community can only benefit from learning about.

"Your Begin/End Transaction model seems to be missing a rollback..."

I used it as an example of some thing with on/off state. Yes, in reality transactional systems usually have two exit routes, so it's a simplified example. Even then, RAII is still cleaner than try/finally, because we can make commit require an explicit call but make rollback be the default, ensure that it always happens if there is not a commit:

using (var transaction = undoSystem.BeginTransaction())
{
    // perform multiple steps...

    // only if we get here without throwing do we commit:
    transaction.Commit();
}

The Commit method stops the rollback from happening on Dispose. Not having to handle both kinds of exit explicitly means that I remove a bit of noise from my code, and I automatically guarantee from the moment I start the transaction that exactly one of rollback and commit will occur by the time I exit the using block.

Case Study: Iterators

The IEnumerable<T> interface inherits IDisposable. If the implementation needs to do something interesting in its Dispose method, does that imply that it should also have a finalizer, to protect itself from users who do not call Dispose?

For an example, look at the most widely used (in modern C#) way of implementing IEnumerable<T>.

When you write an iterator (a function returning IEnumerable<T> and utilizing yield return or yield break), the compiler writes a class for you which takes care of implementing IEnumerable<T>. It does do something important in its Dispose, and yet it does not have a finalizer.

The reason is simple. The Dispose method executes any outstanding finally blocks in the iterator code. The language implementors realised that it would be better for the finally block to never run than for it to run on the finalizer thread. This would have required anything called from finally blocks in iterators to be thread safe!

Fortunately, most clients of IEnumerable<T> use foreach, which works exactly like a using statement - it calls Dispose for you. But that still leaves the cases where the client needs to directly control the enumeration. They have to remember to call Dispose. In the event that they don't, a finalizer cannot be assumed to be a safe fallback. So the compiler does not attempt to solve this problem by adding a finalizer.

Ultimately, this is just one (very widely used) example that demonstrates that there is a class of cleanup problems for which lazy cleanup (GC, finalizer thread) is not applicable. This is why using/IDisposable was added to the language - to provide a purely deterministic cleanup pattern - and why it is useful in its own right in situations where a finalizer would be the wrong choice.

This is not to say that you must never add a finalizer to something that is disposable, just that finalizers are only appropriate in a subset of cases.

[1] http://incrediblejourneysintotheknown.blogspot.com/search/label/IDisposable

I like the counter-points on exceptions. catch(Exception) is sometimes a very valid thing to do. This is a long one! Some refactoring into 2 answers would have been good! - Scott Langham
(2) +1, got to say I recall the lobbying about adding idispose, the hassle from the MS boys who thought a GC was the answer to all resource problems was an eye-opener. I'm not sure they quite get it today. - gbjbaanb
(2) Regarding "x = y can never throw": two corner cases where that is not true (and the world is not falling of its axis) include (1) when the type of y has an implicit cast operator to the type of x, and (2) when y is a property (or, of course, any other method call). - P Daddy
Your advice to ignore the conventions prescribed to by the majority of the industry, as well as Microsoft themselves, sounds a lot like thedailywtf.com/Articles/…. - P Daddy
(4) IDisposable is not RAII. The using statement, as convenient as it is, is not meant as a generic scope guard. The D language has a very handy scope(exit) statement (as well as scope(success) and scope(failure)), but C# is neither D nor C++. C# doesn't have scope guard statements (except for (cont.) - P Daddy
(1) ...try/catch/finally) like D, and it doesn't have deterministic destruction like C++. - P Daddy
Your Begin/End Transaction model seems to be missing a rollback. Rather than make an IDisposable class so that you can do using(BeginTransaction), you'd be better off with BeginTran;try{dostuff; success=true;}catch{Rollback}finally{if(success) Commit}. It's longer and not as nice to read, (cont.) - P Daddy
...but it's more correct. - P Daddy
@P Daddy - I've answered your comments in an edit above, also clarified the assignment-doesn't-throw assumption. - Daniel Earwicker
My comment about advising to ignore conventions didn't refer to your exceptions advice. There is plenty of debate about wrapping vs. not wrapping exceptions (some within this very post) which I'd rather not get in on, except to say that wrapping an exception is definitely a good thing if and only... - P Daddy
...if you add value. This can be by making the message more descriptive, or by, as in your example, changing the exception type to fit your interface. Your exceptions argument starts to sound a little like the checked vs. unchecked exceptions debate, which I'd rather just stay out of. - P Daddy
What I was referring to was your advice to ignore others' advice about using finalizers in the disposable pattern. There's a good reason to use finalizers in [some, but not all] IDisposable implementations, which I think we've covered fairly completely in the comments to Kev's answer in this post. - P Daddy
+1 for your 'rollback if not committed' transaction implementation, though. That is a definite improvement. - P Daddy
It's not suitable for a public API, though, because you rely on using. Whenever you implement an disposable class, you must always assume that consumers of your class (which may even be you on a bad day) will forget to dispose of it (including forgetting to wrap its use in a using statement). ... - P Daddy
One thing we can definitely agree on is that SO should allow longer comments! - Daniel Earwicker
... this is why finalizers are absolutely necessary for any state that won't be cleaned up by the GC. - P Daddy
(1) That argument about consumer forgetting to Dispose, if I took it seriously, would destroy all programming techniques ever invented. Absolutely any library design is vulnerable to consumers who forget to call the right method... - Daniel Earwicker
Agreed on the comments thing! There's a suggestion at uservoice for that: stackoverflow.uservoice.com/pages/general/suggestions/…, but it doesn't appear to be going anywhere. - P Daddy
But if you name your method IDisposable.Dispose(), the language helps you to call it at the right time (just before the object that implements it disappears). So it's better than ANY other way of designing a library that requires a predictable clean-up callback, even if it's still not perfect. - Daniel Earwicker
re: "would destroy all programming techniques". That's baloney. There are many programming techniques that were invented simply because people screw up. Exceptions were more or less an answer to programmers neglecting to check error codes. The IDisposable pattern itself, along with RAII, ... - P Daddy
...are answers to programmers neglecting to release resources. People are imperfect. I am imperfect. You are imperfect. You must code defensively against people's imperfections. - P Daddy
Note that in C++ the same problem exists. A coder may say std::vector<int> *p = new std::vector<int>(1000000), and be surprised that it doesn't reclaim the MB of memory when p goes out of scope. The exact equivalent of forgetting 'using'. - Daniel Earwicker
Hmm... maybe you're missing my point. I suggest using Dispose as the name for a clean-up method. You say "that's no good, what if they forget to use using?" I say: what if I call my method something else, and they still forget to call it? Hence your criticism has no force against my suggestion. - Daniel Earwicker
"the language helps you call it at the right time (just before the object disappears)": Huh? What you're describing is a feature of the GC, not the language, and is an argument for having a finalizer. - P Daddy
Note that in C++, memory leaks (like the one you show) are the #1 source of bugs. - P Daddy
Obviously I don't deny people screw up if left to their own devices - that's precisely why I recommend using over try/finally. It makes it just a little bit more clean, automatic and hard to get wrong, which is always a good thing. - Daniel Earwicker
Dispose absolutely should be the name of a cleanup method, and is why this pattern exists. However, you suggest relying on consumers to always call it, and I'm insisting that you can't. Like it or not, not everybody is as infallible as you or I. By not everybody, I mean you or me on a bad day, - P Daddy
...or our less-skilled coworkers who must use our class, or the people we sell to whom we sell our libraries. - P Daddy
And you must know I'm talking (throughout) about the using statement, not the GC. I'm starting to wonder if you're just looking for ways to misinterpret whatever I say! It's the same with that comment about C++ memory leaks - how does that address my point? - Daniel Earwicker
There are basically two reasons for the disposable pattern. One is to provide for early release of limited resources, or otherwise early cleanup of state. It's great when a well-behaved programmer calls Dispose as soon as he's done using an object. The other reason is for a limited guarantee... - P Daddy
...that resources (or state) will be cleaned up eventually, even if the consumer is not well-behaved. This is where the finalizer comes in. If we could always rely on users to call Dispose, then we wouldn't need to add finalizers, ever. But then, we wouldn't need to disposable pattern at all. ... - P Daddy
... Each class would have its own cleanup method, like File's Close (which File.Dispose calls). - P Daddy
I'm not relying on consumers to always call anything. I can't even begin to see how you've reached that interpretation. - Daniel Earwicker
I don't think I'm misinterpreting you at all. You said "And ask yourself - would it make much sense for UndoTransaction to have a finalizer as well? Absolutely NOT." I've directly addressed this. About your C++ memory leaks comment: just what was your point? - P Daddy
You appear to arrived back at the idea that Dispose REQUIRES a finalizer. You just aren't reading what I'm saying, and you aren't reading current advice. An IDisposable object does not always need a finalizer, and as such it is reliant on being used correctly (as is any class!) - Daniel Earwicker
My point with the 'new std::vector' was to provide another example of how you cannot stop users from abusing your class. They can always use it wrong. This is not an argument against providing them with an easier way to use it correctly. - Daniel Earwicker
Yes, you're relying on consumers to always call Dispose (either directly, or indirectly though a using). Not that, for example, wrapping your transaction in the disposable pattern is a bad thing, but without guarding against user non-compliance to the pattern, you've added little value. - P Daddy
(2) If UndoTransaction had a finalizer, that would mean that the undo transaction would get ended at a random time by the finalizer thread. That would actually be worse than it never being ended, as it would be harder to detect and the results would be non-deterministic. - Daniel Earwicker
We're getting somewhere - you agree I've added some "little value". In fact I've added precisely the value of the using statement itself. It's in the language for a reason - for that "little value" it provides. - Daniel Earwicker
You're missing my point, then. A user forgetting to free something in C++ that he has allocated himself is a user error that you can't do anything about, but it's not your responsibility, unless you're the user that did it. A user forgetting to free something that your library returned to him ... - P Daddy
... is a user error that you can do something about. You can return the wrap the memory in a smart pointer, so that it will be freed automatically as soon as the user's pointer goes out of scope. Similarly, if you provide a finalizer with your disposable object, you can prevent user errors... - P Daddy
... in C# when users forget to call Dispose. - P Daddy
"If UndoTransaction had a finalizer ... the undo transaction would get ended at a random time": only in the worst-case scenario where the user neglected to Dispose of it properly. But ending at a random time is better than not ending at all! - P Daddy
Er.. you prefer programs with random behaviour? I wish you good luck, and adieu. - Daniel Earwicker
That's ludicrous and argumentative. What I do prefer is eventual finalization to none at all. Of course, I'd prefer my users to be perfect and never miss a Dispose call, but if everybody were perfect, then this post would have no place! - P Daddy
Your response is "It would be better if your library attempted non-deterministic cleanup in the eventually of a bug in the user's code." You have to accept that this might be a disastrous thing. You have to accept that there are many situations where this would make the bug harder to track down... - Daniel Earwicker
(1) If you can't accept that, we'll just be going round in circles forever. It's the crux of what I'm saying. There are situations where an attempt at non-deterministic cleanup is worse than no cleanup. Specifically, any system of states that work like a stack. - Daniel Earwicker
In such cases, the bug can simply be detected by noting that the stack is not emptying correctly, and the program halted with an exception saying "You forgot to dispose" so the user can fix the bug. - Daniel Earwicker
This is where the rest of the industry disagrees with you. Late cleanup is better than no cleanup. See msdn.microsoft.com/en-us/library/b1yfkh5e.aspx or bluebytesoftware.com/blog/2008/02/18/…. - P Daddy
(1) Have you actually read the second link? "We may or may not want people to implement a finalizer to do the same [as Dispose]. I currently believe we will." It's an open question, not a thing with expert consensus at all. So I'm afraid you're going to have to think about it, not blindly follow rules. - Daniel Earwicker
Late cleanup is better than no clean up - except when it would cause non-determinate side-effects and make the bug harder to find. How can I make it simpler than that? You're blindly applying a rule that is not always applicable, that's all. - Daniel Earwicker
The point about wrapping exceptions is interesting - but do you mean that I should wrap every single public method in a try-catch block? Is it not far simper and less error-prone for the user of the class just to expect any exception to be possible, and handle specific ones as appropriate? - Joel in Gö
@Joel - my point really is that there is no single correct answer. It depends what level of binary compatibility you want to offer the caller of your code. The language supports filtering exceptions by type, hence exception types are part of the binary interface... - Daniel Earwicker
(1) ... but you could document that "all bets are off" when it comes to exceptions thrown by your method. I increasingly believe that there should only be two exception types that a 'catch' can specify: Fatal and Recoverable. - Daniel Earwicker
If a Fatal exception leaves a method, the caller should attempt to unwind whatever they were doing and propagate the exception. For a Recoverable exception, they can unwind and either "swallow" the exception (e.g. display a message, log, continue) or propagate it. - Daniel Earwicker
But callers should never second guess how to recover based on information in the exception. So beyond the fact that the exception is either fatal or recoverable, the caller's code path must not depend on information in the exception. Wrapping is potentially a way to enforce this. - Daniel Earwicker
(22) -1 this answer is just too long - finnw
@finnw - that's okay, just stop reading as soon as your brain starts to hurt. - Daniel Earwicker
hmm, I didn't mean to upvote Earwicker's comment. I hit the arrow by accident. On an unrelated note; I love it that I can say "using (RWLock.ForReadingOnly) { ... }" in C#. It now becomes so much easier to see when the lock is taken and released and what part of the code makes use of it. In this case RWLock is a class I wrote that wraps an ReaderWriterLockSlim and has three properties that return a disposable to get a read, upgradable read or write lock from the ReaderWriterLockSlim. - Patrick Huizinga
+1 for GetFunky(); - tsilb
Regarding your use of Dispose for general RAII, it looked like a good idea at first but now I'm thinking you're probably always better off using a higher-order function that guarantees the initial and final code is run. - Jon Harrop
(1) @Jon Harrop - Definitely, that's what I usually recoomend now, e.g. stackoverflow.com/questions/2617400/synchronized-ienumeratort/… - it's not always possible though because sometimes you need to be able to compose resources. - Daniel Earwicker
(1) This is the longest answer I've ever read on any forum. - kirk.burleson
(1) @finnw not a -1, but it would have helped to split this long comment into smaller ones. I might agree on one of the parts but not on all. - Lars
13
[+105] [2008-12-19 17:50:45] MatthewMartin

Unnecessary initialization,

DataTable foo = new DataTable();  // Initialization unnecessary
foo = FetchDataTableFromDatabase();

Better:

DataTable foo = null;
foo = FetchDataTableFromDatabase();

Also fine,

DataTable foo = null;
try {
     foo = FetchDataTableFromDatabase();
}
catch { 
     //do something
}

Best,

DataTable foo = FetchDataTableFromDatabase();

However, you can't blindly remove those constructs. What if the DataTable constructor contains side effects? It might be there for a reason. Though, I would agree that this is bad practice. - Tom Lokhorst
Wow, that is an interesting edge case. You mean the original programmer intended the side effects of the constructor to happen twice and wasn't just copying a common COM/VB6 construction pattern? Unnecessary init causes the constructor to run twice, and the results of the 1st are discarded. - MatthewMartin
(26) Your second case is necessary if your "Fetch" needs to be in a try/catch, and your Foo needs to be declared out of the try/catch scope. In that case you would not be able to use your third case. - DevinB
(30) best: var foo = FetchDataTableFromDatabase(); - Arnis L.
Agree! It gets on my nerves and denotes the developer doent know much about the language/paradigm - graffic
I have code to maintain (not my code originally) that uses the first construct to ensure the result is never null. Of course you still need to see if .Tables.Count > 0 anyway. - Mark Hurd
(16) @Arnis - I disagree for most cases. IMO, var is great for locals within methods where you can see what type it is, but with your example you can't tell what the return type is of that method. - Paul
@Paul those are Your rights. I did believe in Types too. - Arnis L.
@Paul "FetchDataTable" isn't clear enough? - Brian Ortiz
@Brian This is example code, I actually almost never embed data types or signature info into the name. I fear the day when people resume writing Hungarian to make code written with var more readable. - MatthewMartin
@MatthewMartin I disagree that it is bad practice. In fact, I believe it is common and nothing like Hungarian notation. - Brian Ortiz
@Mark: The first construct doesn't guarantee anything about nullness, because the method could return null. In all cases, the first initialization is a waste. - Mark
@Mark: Good point. I really mean something more like a combination of the first and third construct. (BTW Will this realise I wouldn't be replying to myself?) - Mark Hurd
14
[+96] [2008-12-20 09:12:07] EMP

Not using ReSharper! (Or another code analysis tool - but R# is the best.)

I'm surprised nobody has mentioned it yet, because it automatically picks up many of the mistakes mentioned in other answers.


(2) I was going to say that! Although gradually VS is picking up the features of Resharper. The Resharper folks are going to struggle to differentiate themselves. (Although for us this is a good thing - the tools just get better and better.) - Daniel Earwicker
Live Templates... VS still has a while to go. - Min
(3) Resharper is good. In one way it pump up Your performance, but greatly reduce overall performance of PC. And it always bring down on knees my work IntelCoreQuad with 4 gigs of RAM. Without R# I feeling more productive and getting more attention on errors by myself. - AlfeG
(25) Not using Resharper is not a common programming mistake. - kirk.burleson
I hope you work for jetbrains (making this answer explainable.. otherwise...) - Andrei Rinea
@kirk - perhaps not a "mistake" as such, more of a sub-optimal development practice. I think the answer was still relevant and useful, however. @Andrei - No, I do not work for JetBrains - but I probably would. :) - EMP
If I didn't consistently see ReSharper slowing Visual Studio down to a crawl and making it crash, then maybe I'd consider using it. - Jacob
15
[+77] [2008-12-22 06:36:27] icelava

Referencing constants across assemblies, that may not get updated together.

Here is an article i wrote in 2007, pasted whole sale

Referencing Constants

We all know the classic lesson from school. Never use “magic numbers” in your code. Always define a constant and use it throughout. Not only does it give it contextual meaning, it also makes it easy to alter the value only at one place in the future. Sweet deal huh? Well, maybe not as much as one might think. There is a subtle issue with the use of constants that perhaps not everybody is aware of. Let’s do something practical to sink the idea; go ahead and open up Visual Studio:

  1. Create a class library project, call it ConstantLibrary
  2. Create a WinForms project, call it ConstantDependent
  3. Let’s imagine for a moment that we’re going to program World of Warcraft all over again. :-)

Those who play WOW know that the maximum attainable player level used to be 60. So let’s create a PlayerLimits class in ConstantLibrary

namespace ConstantExample
{
    public class PlayerLimits
    {
        public const int MaxLevel = 60;
    }
}

Now, in ConstantDependent, 1. Use Form1 2. Put in a button btnMaxLevel 3. Put in a label lblMaxLevel 4. Set the btnMaxLevel click event to

private void btnMaxLevel_Click(object sender, EventArgs e)
{
    this.lblMaxLevel.Text = PlayerLimits.MaxLevel.ToString();
}

Build and run the solution. When you click the button, 60 appear. Now,

  1. Go back and adjust PlayerLimits.MaxLevel = 70, the new level limit introduced in The Burning Crusade expansion.
  2. Build only the ConstantExample project, and copy its new assembly to ConstantDependent’s bin/Debug directory to overwrite the old assembly.
  3. Run the ConstantDependent.exe that is directly there; make sure you did not recompile it.
  4. Go ahead and press the button again.

It remains at 60. Oops. What is happening here?

  1. Launch MSIL DASM, the disassembler supplied with the .NET Framework SDK.
  2. Load ConstantDependent.exe into it.
  3. Look for method btnMaxLevel_Click and open it up, and look at the line with the ldc instruction to load an integer value onto the stack.

To be specific, it would be IL_0007 in the sample below.

.method private hidebysig instance void btnMaxLevel_Click(object sender, class [mscorlib]System.EventArgs e) cil managed

{

  // Code size       24 (0x18)

  .maxstack  2

  .locals init ([0] int32 CS$0$0000)

IL_0000:  nop

IL_0001:  ldarg.0

IL_0002:  ldfld      class [System.Windows.Forms]System.Windows.Forms.Label ConstantExample.Form1::lblMaxLevel

IL_0007:  ldc.i4.s   60

IL_0009:  stloc.0

IL_000a:  ldloca.s   CS$0$0000

IL_000c:  call       instance string [mscorlib]System.Int32::ToString()

IL_0011:  callvirt   instance void [System.Windows.Forms System.Windows.Forms.Control::set_Text(string)

IL_0016:  nop

IL_0017:  ret

} // end of method Form1::btnMaxLevel_Click

The IL code is using the literal integer value of 60. Ouch. What the C# compiler has done is to inline the constant value literally into the client assembly. If you are in one of those environments where you are only allowed to promote changed assemblies into UAT or production environment, and you thought you could alter just an assembly with modified constants, well, we all thought wrong.

Recommendation: Use constants only within an assembly. If they are placed in some other assembly, make sure they get compiled together and promoted together, even when the client assembly has no change in code. If you can guarantee the constants never change values, then power to you. Otherwise, use static read-only values for dynamic referencing. The following snippet will “propagate” the correct value to the client assembly even if it wasn’t compiled together.

public class RaidLimits
{
    public static readonly int MaxPlayers = 25;
}

How about wrapping constanst into read-only properties? Does that get inlined in the client assembly too? - DrJokepu
(1) Would not you use readonly right from the start, then? :-) - icelava
(12) +1 for very informative and good investigated answer. - BeowulfOF
Very good answer! I always use internal constants unless the constant should never change (I comment it that way too). Otherwise I use readonly members. - Peter Morris
(5) I think that constants is just for constants. Not number of player. Example: PI, Plank and others math consts, speed of light, int.MaxValue.... - Fujiy
It's definitely important to be aware of this, but constants have one big advantage over static readonly public variables: you can switch on them. - Jeff Sternal
+1 This is very infomative and clear explaination! - Emrah GOZCU
@Fujiy this is just an example to illustrate the point clearly. unfortunately in real business world, constants are NOT strictly used for mathematical/scientific values. - icelava
@icelava, I agree, anyway const should be used strictly to natural things, that never change, in business rules this is rare. - Fujiy
+1 for bringing WoW into the topic and -1 for being wrong about the maximum level... It's set into the server not the client... - Vercas
(1) erm, this is just an example not a reenactment of WoW source code... - icelava
Does this also apply to enum or just const? - Chris Moschini
(1) @ChrisMoschini yes, enums in .NET are compiled into a struct derived from System.Enum, with each enum item turned into const of the underlying value type (i.e. int32). So that const value gets copied and pasted into the consuming assembly. if the values of enum items are ever changed, consuming assemblies that were not recompiled with the enum assembly will continue to use the old value, resulting in them displaying the wrong string value when dereferenced. - icelava
@icelava If the assembly version number increases when the enum changes, will this prevent this from happening? This seems like a major .Net Framework flaw - enums are a useful and important tool, especially in reusable libraries. - Chris Moschini
(1) @ChrisMoschini if your consuming assembly has a strong-name reference to the old assembly, it will fail to load (assembly not found) even if the new version is there. Unless there is an assembly-redirect configuration to point the old version to the new version. - icelava
@ChrisMoschini I'm not too sure if this is really a "flaw" as it is a design that is misunderstood by developers due to some preconceptions. At any rate, it is essentially never a good choice to renumber Enum items; there could be more things than .NET code that rely on those values being fixed, like database records. - icelava
16
[+69] [2009-03-12 01:29:59] Anthony Brien

If you know in advance the size of collection you are about to fill, reserve the space when creating your collection.

List<Person> persons = new List<Person>(listBox.Items.Count); // Reserve
foreach (ListBoxItem lbi in listBox.Items)
{
    persons.Add(lbi.Tag as Person); // No reallocation
}

For very large list, not reserving the space causes the collection to be re-allocated over and over (at each power of two).

Another tip: When adding a lot of items to a collection, it's more efficient to use one AddRange instead of a sequence of Add. This is especially true with observed collections like the Items collection on a ListView.

foreach (string line in File.ReadAllLines(fileName))
{
    // Not the best: there might be overhead when the collection changes
    // and multiple reallocations 
    listBox.Items.Add(line);
}

// Much faster: Single call, minimal overhead, and only one 
// potential reallocation
listBox.Items.AddRange(File.ReadAllLines(fileName));

(1) Have you done any testing to indicate that reserving space in the collection this way speeds things up significantly? - Kyralessa
(2) (Agreed on the AddRange, though, which in WinForms is often essential to keep your app from slowing to a crawl.) - Kyralessa
(1) It remember after I introduced a Reserve(), it did improve my performance a lot according to dotTrace (a .NET profiler). But it was for a specific example where the collection was huge (over 100,000 items). As your collection grows very large, the cost of resizing the array (e.g. reallocating it and copying all elements to the new one) becomes bigger and bigger. So if you expect a collection to be very large, and you know the size in advanced, its worth using Reserve. If you have a small array with under 100 items, you probably wont notice a difference. - Anthony Brien
Upvote for space reservation thingy. - Arnis L.
+1 for space reservation thingy haha - SwDevMan81
wow...since I started programming with WPF and not winforms I completely forgot about AddRange! Thanks for the reminder :) - townsean
17
[+69] [2010-09-18 21:05:16] Rei Miyasaka

Believing that you're improving array iterating loop performance by extracting the Length property ahead of time.

Surprisingly, this is actually slower:

int[] values = int[10000];

int length = values.Length;
for (int i = 0; i < length; i++)
    values[i] = i;

Than this:

int[] values = int[10000];

for (int i = 0; i < values.Length; i++)
    values[i] = i;

The .NET runtime injects a check each time you access a value in an array, to make sure that the index you've given it is >= 0 and < Length. This eliminates a whole slew of horrifying security flaws known as buffer overflows [1], and is one of the main selling points of so-called "managed languages" like C# or Java or Python over an unmanaged language like C/C++.

On the other hand, of course, this impedes performance to some degree (usually not much). .NET alleviates this by making an exception for certain cases. In loops like the one above, where the each value from [0] to [length-1] is being accessed, the check is ommitted, and the code run as fast as if it were an unmanaged language.

But this optimization can't be performed if Length is copied to a local variable first, because it has no easy way of knowing that your variable is correct.

[1] http://en.wikipedia.org/wiki/Buffer_overflow

(6) +1 Wow, I did this all the time! - vash47
I recently found this out as well and it blew my mind. I think this is hard for the old C++ devs out there who are used to doing this kind of thing. The compiler actually does write the most optimized code for you. - Justin
I was getting blasted (-1's) for using a list.Last() check, because Last() does a loop through the entire list for the last element each time, so no such optimization exists for that case? They said I should do what you are saying not to do, get the last value first, and then check against that variable of last. - Chuck Savage
Last() is actually quite different, and yeah, it has to go through every item. The Linq functions are all sequential, which means they work like a tape deck -- start from the beginning and fast forward to wherever you want to get to. Believe it or not this has advantages (especially if you're accessing every item in the list, which is often), but I definitely wouldn't recommend doing Last() on arrays. It sucks that the documentation doesn't really give you the performance characteristics, but in technical terms, Last() is an O(n) operation -- that is, it takes as much time as there are items. - Rei Miyasaka
This is actually not true. I did a test of this myself. The IL generated for test 1 above is larger than that generate for test 2. However, if you were to reverse these in a Console app so that test 2 ran before test 1, then you will see different results. Also try running this for 100000000 iterations. There is a difference. - AboutDev
@AboutDev Try setting your compilation target to x64 if you're on an x64 machine. That seems to make the results more consistent, with the first test being very marginally slower whether it's done first or last. Microsoft has been doing some really bizarre things with having the x86 and x64 JIT code having a different set of optimizations. x86 supports inlining functions with struct parameters whereas x64 doesn't, and x64 supports tail call recursion optimization whereas x86 doesn't. It's really quite frustrating. - Rei Miyasaka
18
[+67] [2008-12-19 20:24:22] Matthew Ruston

I've caught myself a few times writing my getter and setter properties in C# incorrectly by referencing the name of the property in the get {} set {} blocks instead of the actual variable. Doing this causes an infinite loop due to the self-referential calls and eventually a StackoverflowException.

Example (Incorrect)

public int Property
{
    get
    {
        return Property;
    }
}

(44) +1 for saying Stackoverflow and meaning it :D - DD59
Heh I learned this one really early in my programming career. 'WTF is going on with this Stack Overflow? My class only has one property so far!?!?!?' - steve_c
This is when tools like Resharper come in handy. It'll put a recursion icon on the side-bar next to the line of code, making it easy to identify accidental recursions like this. - Spodi
Spodi, definitely, resharper! Also, I tend to do "this.property" when referencing instance member variables. - BobbyShaftoe
(8) Let's hear it for autoprops, that make this problem less likely to occur. - Jay Bazuzi
(8) Would be nice if the compiler generated warnings for this! - Dan Diplo
(2) I think everyone has made this mistake at least once. - Chris Cudmore
A coding convention, like using underscore as a prefix of private fields, helps to avoid this kind of issue. In addition, you'll have the convinience of typing _ on the code editor and Intellisense will show up all your private fields together. I know it is a matter of coding style/convention, but for me it is useful. - Carlos Loth
After the first time this one is completely obvious. - kirk.burleson
it happened to me yesterday :)) - DarthVader
public int Property { get; private set; } is one way to do the same thing from C# 3.0 onwards. - ydobonmai
19
[+57] [2008-12-19 12:23:19] amazedsaint

1 - Some of us don't use using. Use using where ever possible

using (StreamReader reader=new StreamReader(file)) 
{ 
 //your code here 
}

2 - Forgetting to check if something is null, instead of trying to catch a null condition when an exception occurs

//This might throw an exception
string val=obj.Value.DomeSomething()


//Better, check for null

if (null!=obj.Value)) { 
 // Do your stuff here 
 }

3- Forgetting to check for null after a runtime type cast

   MyType t= x as MyType;
   if (null!=t) { 
      //Do stuff here 
    }

4- Where ever you are allocating something in try block, make sure you've a finally block to release stuff.


(4) If obj.value == null, then obj.Value != myval (unless myval == null), so your extra check is unnecessary. Also the if(rhs == lhs) pattern (e.g. null == x, instead of x == null) is unintuitive and unnecessary in C#, since if(x = null) is not valid in C#. - P Daddy
I think you might really want to change the first if condition to: if(obj != null && myVal == obj.Value) ... - BobbyShaftoe
Clarified the intent of the comment - amazedsaint
(2) May I ask why you opt for null != x as opposed to x != null? Seems like a Yoda Condition if you ask me! - David Foster
@David I was going to say something similar about it! Never saw that before :D - Oscar Mederos
Don't use using with WCF, or other objects that won't properly dispose of event handlers when an exception occurs. See this resolution for WCF and this resolution for Event Handlers - makerofthings7
(1) Simple answer to that one: if (x = 5) {} is potentially valid code. if (5 = x) {} is not. - Flynn1179
if (x = 5) {} raises the compiler error Cannot implicitly convert type 'int' to 'bool' for precisely this reason. It's simply a habit some people are in from the unmanaged languages where such problems occurred. - Kevin Fee
20
[+53] [2008-12-20 09:56:38] Echiban

Violating standards or conventions without knowing why they are there, or worse, refuse to even acknowledge their value.

It makes their code hard to read, hard to re-use.


(4) This needs more upvotes. - Tordek
(10) This needs an example. - kirk.burleson
@Tordek: It has to stay at 42 votes, and you know why. All awesome questions and answers don't have 80,000 upvotes but precisely 42. - JavaAndCSharp
lets quickly make it 43 then :P - Levisaxos
21
[+48] [2008-12-19 12:23:56] joseph.ferris

I am sure that I have more. These are more like my current "Top Pet Peeves":

  • Not properly disposing of disposable resources. The using() keyword should be used with every object that implements IDisposable. Considering the code compiles to the equivalent of a try / finally that properly disposes of the object provides cleaner and safer code.

  • Catching Exception instead of a specific exception. Not only that, but seeing code where every single method has the entire body inside of a big try / catch. You should only ever catch exceptions that you can handle and are expecting. Add a top level handler to catch unhandled exceptions.

  • Seeing exceptions used to control program flow. If an exception is thrown, it should not be swallowed or used to initiate another set of logic that would not happen for any other reason.


"compiles to the equivalent of a try / catch" - a try/finally actually. - Joe
Yes, Joe. Thank you. I'll update the post! :-) - joseph.ferris
(11) Catching Exception is fine as long as it is only for logging purposes, and you are going to re-throw (throw, not throw ex). - Marc Gravell
@Marc - Yes. That is why I said that you should use a top-level handler for unhandled exceptions. - joseph.ferris
But in some cases that generate a lot of unknown exceptions (when you're interacting with external data) you will often need the context of what you were doing at the time (use a local try/catch) but not know what exception it will throw (catch a general exception) - DevinB
Yes, but that is why Microsoft provides inheritence in their exception model, as well. You don't need to know the exact exception all the time. For example, FormatException can be caught and catches FileFormateException, CookieException, etc. There are scenarios, but they are uncommon. - joseph.ferris
Catching the general exception "generally" is fine as long as it's handled at the topmost level of the code (IE the main method, top level UI event handlers). Also catching the general exception should always be followed by logging or displaying an UI message whiteout continuing execution flow. - Pop Catalin
(4) Using exceptions to control program flow isn't bad idea. Exception IS NOT an error, exception is just NOT-STANDARD behaviour. Don't afraid exceptions. I had situations where using exception made code 100 times more elegant than checking returned value for null and then checking value itself. Here's simplified example. Your method must parse file to some data hierarchy. Throw an exception if the file wasn't found or if content of file didn't match expected format (maybe different exceptions). If would be much more clearly than returning null. Of course your exceptions must have meaningful type. - Dmitry Lobanov
22
[+43] [2009-04-04 15:38:33] Fredy Treboux

A common error when trying to create a central exception handler on Windows Forms [1]:

try
{
    Application.Run(someForm)
}
catch (Exception ex)
{
    //This won't catch your Windows Forms exceptions
    //(even if inside Visual Studio it does).
}

You should use this for the behaviour you want:

// Add the event handler for handling UI thread exceptions to the event.
Application.ThreadException += someThreadExceptionEventHandler;

// Set the unhandled exception mode to force all Windows Forms errors to go through
// our handler.
Application.SetUnhandledExceptionMode(UnhandledExceptionMode.CatchException);
[1] http://en.wikipedia.org/wiki/Windows_Forms

23
[+41] [2008-12-19 13:14:57] Mendelt

Locking on this is always a nice one. Not immediately fatal but a good way to get deadlocks.


assuming "this" is a public type... just sayin'... - TheSoftwareJedi
yep, use System.Threading.Monitor.TryEnter() and System.Threading.Monitor.Exit() instead. - vitule
why use monitor when you can lock on a private object? - Jimmy
I always create an individual readonly private object to lock on for each logically-separate lock. With Monitor.TryEnter()/.Exit(), it can be easy to fail to unlock when intended, especially when an exception is thrown. - Spodi
Agreed. A shame that [MethodImpl(MethodImplOptions.Synchronized)] and field-like-events both do exactly this. - Marc Gravell
afaik lock actually uses Monitor.Enter and Monitor.Exit. I tend to use Monitor directly because this enables you to do use TryEnter and Wait too. - Mendelt
I don't lock(this), but I don't see why it would cause a deadlock any more than lock(SyncRoot) - Peter Morris
If you lock on something that's publicly accessible (Microsoft uses lock(this) a lot in examples but locking on a publicly accessible SyncRoot is just as bad) you can easilly get deadlocks when other code locked the same object. - Mendelt
(2) Got advice from my concurrency teacher that public synchronized void M() { /* mod internal state */ } is completely acceptable and he got verbally angry with me for even dragging up such a stupid issue not affecting real code (obviously this is the same as locking on this). Don't go to Imperial College Computing, they don't know their stuff :P. - Henrik
(2) lock(this) isn't bad when you don't lock(that) - oɔɯǝɹ
24
[+41] [2009-08-21 19:57:18] Emrah GOZCU

Checking a string variable if it is assigned. String object can be null so;

Wrong!,

if (someStringVariable.length > 0) {
    //Do you always get this far?
}

Correct,

if (!String.IsNullOrEmpty(someStringVariable)) {
    //Yeah! This one is better!
}

(7) +1 Good answer, this is definitely good to know - SwDevMan81
(4) You can get even better by using string.IsNullOrWhitespace in some cases (if .NET 4 is used) - Andrei Rinea
(1) Valid in C# not in VB.Net. In VB (someStringVariable = "") is True when someStringVariable is null, it doesn't throw. - MarkJ
25
[+35] [2008-12-19 13:24:44] TheAgent

One of the most dangerous pitfalls:

Creating a temp object, using its events by utilizing AddHandler (in VB) and then forgetting to remove those handlers. One would think that the object is collected by Garbage Collector when it goes out of scope, but it won't since there is still a pointer to that object (a function pointer) and GC won't clean it up.

You will also notice that the event handler hits many times. Once for every object you've created, used its events, and forgot to remove it. In addition to memory problems, this would cause your app to run slower and slower while it is working because the code in your handler would execute multiple times.

Just realized this problem because of performance issues of my app.


+1 this is good advice, and is often overlooked, IME. - hmcclungiii
This has caused us some serious issues in the past. Can't upvote it enough. - Jeffrey
Been there, done that! And I have learned from the experience. - Stefan
26
[+33] [2008-12-19 15:41:49] Joe

Like, trying to open a file without checking whether it exists ...

This is not necessarily a mistake. If you know the file ought to exist (e.g. a configuration file, or a file name obtained using an OpenFileDialog), it's often perfectly OK to just go ahead and open it, and let any exception propagate.

And checking for existence doesn't guarantee it will still exist when you try to open it.

It may make sense to check if you're opening a file in the presentation tier - where you can, for example tell the user the file doesn't exist.

But in the business tier, what are you going to do if the expected file doesn't exist?

  • Throw a FileNotFoundException? In which case you might as well just try to open the file.

  • Throw a custom exception? In which case callers will need to be aware that either your custom exception (for the common case) or a FileNotFoundException (if the file disappears between checking and attempting to open) - which potentially adds complexity.


Joe is correct: Its can be extra work to check for existence before opening. For example, you existence call could succeed, then the file be deleted before your open call which will then fail. The value here is that it can be a tad more effient to check. But its not a correctness issue. - Foredecker
Foredecker ... oddly, a very good point! - BobbyShaftoe
(4) Raymond Chen has written on this issue: blogs.msdn.com/oldnewthing/archive/2007/11/09/6001644.aspx - Eclipse
(1) Personally, I'd check (which will cover 99% of cases) but also handle the exception (to cover the 1% of cases where the check succeeds but the file disappears immediately after). - Kyralessa
Here's a great article that talks about what kinds of exceptions you need to handle. IOException is one of them. blogs.msdn.com/kcwalina/archive/2007/01/30/… - Kyralessa
27
[+30] [2008-12-19 22:50:45] Matthew Wright

Use FxCop to pick up on common coding mistakes. Some of the things it picks up on are a bit trivial, but it has helped us pick up a number of bugs which might otherwise have been missed. Run it from Visual Studio, Analyze->Run Code Analysis for ..., or be really good and set it up to run every time you do a build in the Code Analysis section of the project properties.


28
[+28] [2008-12-19 13:47:34] nruessmann

Change the name of a property without carefully checking if it is used in data binding.

Properties are used for databinding. Unfortunately the binding mechanism in Windows Forms and WPF use the property name as string. If you change the name of a property, you will not get any compiler error or warning, only a runtime error if you are lucky.


(11) shhh .. we're all supposed to believe in the dynamic language propaganda and eschew any nonsense about compiler errors. :) - BobbyShaftoe
Doesn't refactoring from the IDE solve this problem? - dar7yl
dar7yl, no it doesn't, because when you bind a property, you refer to it by name, in a string. IDE refactoring won't even replace literal strings, and of course they don't even have to be literals anyway. - Blorgbeard
The same with object data source in webforms. I guess there have to be workarounds through extension methods. - Arnis L.
The rename refactoring in Visual Studio can be set to look in strings. Which isn't an ironclad guarantee, but helps, at least. - Kyralessa
(2) a solution to this issue, here : stackoverflow.com/questions/1329138/… - Toto
29
[+28] [2008-12-19 21:36:16] rizzle

Use generics collections (List<T>) instead of ArrayList so that you can maintain type safety.

re:using
Don't unnecessarily nest using statements do this instead:

 using (SQLconnection conn = new SQLConnection() )
 using (SQLCommand cmd = new SQLCommand("select 1", conn)) // no need to nest
 {
    conn.open()
    using (SqlDatareader dr = cmd.ExecuteReader()) //nessesary nest
    {
      //dr.read()
    }     
 }

Why mark this down? Both good points. - Daniel Earwicker
(5) A lot of style guides would frown on this because the first using isn't using brackets. It's mostly an issue of preference...but I think it's better to have the brackets. - Beska
(14) I would still call that nested, even if there are no brackets used. - Neil Whitaker
It's "better" to just use C++ in the first place where RAII is done right! ;) - Eclipse
That is still nested, and VS will indent it as such when you put the closing brace on your method. - Blorgbeard
I already forgot that such a thing like ArrayList exists. And it's ain't bad NOT to use brackets, if code is readable. - Arnis L.
Logically it's nested, sure, but visually it's not nested. The point of this style is to avoid visual nesting, which clutters the code for no good reason. - Kyralessa
(1) I wish that the using statement would allow for something like this, it would solve most of the readability issues: using (Resource res = new Resource(), Wrapper wra = new Wrapper(res)) - Allon Guralnek
This is a post that would get closed if it were a question. It's akin to the bracket/indentation debate. - Rei Miyasaka
Akin? IMO it's the same. I like explicit brackets on if, and "then" statements in next line - and the same for using. - ANeves
@Allon This is perfectly legal VB.Net Using (Resource res = new Resource(), Wrapper wra = new Wrapper(res)) C# is so verbose ;) - MarkJ
30
[+26] [2009-01-03 17:39:20] Tom Moseley

Putting bad execution code in Get accessors

If you have code that modifies an objects state in its Get accessor, then examining that property in the debugger, which causes the code to execute, can alter the state of your object.

For instance, if you have the following code in a class...

private bool myFlag = false;
public string myString
{
  get
  {
    myFlag = true;
    return "test";
  }
}

The first time you run into this in the Debugger, myFlag will show as having a value of false, and myString will show as having a value of "test". If you hover over myFlag, however, you will see that its value is now true. The debugger's display of the property value has changed the state of your object.


Very interesting. Not something I would hope happens often in a getter, more likely to happen in a setter, but interesting none the less. - Ray Booysen
Modifying an object state in a property getter is not a problem as long as the state change is idempotent. In your example it is indeed idempotent, so you have not demonstrated a programming error. Of course, it is still possible to write crazy getters, but I don't think that is very common. - Timwi
@Timwi I don't know what you mean can you explain. In this getter if it's examined in debug mode it will cause the state of myFlag to change when it might not have in the program. This could affect the program later based on what myFlag is used for. - PeteT
(4) @petebob796 It is difficult to explain the concept of idempotency in just a few words. Of course the change to myFlag could affect the program, and if it does, the program has a bug — but the bug is not necessarily in the property getter that changes myFlag. Consider, for example, a cache: a property retrieves a value and then places it in a private field for faster retrieval in future. This can certainly have an effect on the program if you use the private field in crazy ways, but that doesn’t mean you shouldn’t ever cache any values — it only means you shouldn’t use caches in crazy ways. - Timwi
31
[+22] [2008-12-19 12:18:25] flesh

Never publicly expose generic Lists - and FAQ: Why does DoNotExposeGenericLists recommend that I expose Collection instead of List? [David Kean] [1] explains why not.

[1] http://blogs.msdn.com/fxcop/archive/2006/04/27/faq-why-does-donotexposegenericlists-recommend-that-i-expose-collection-lt-t-gt-instead-of-list-lt-t-gt-david-kean.aspx

(4) Also, use IEnumerable<T> where appropriate (read only collections). It's always best to stick with the lowest interface possible. - TheSoftwareJedi
Good point, I never thought about that. - unforgiven3
You have to keep in mind a few things about the framework guidelines. They are designed for APIS used by millions of developers, where the inconvience they generate makes up for the massive expense that would occur if they were violated. Most classes written by most programers are ... - Scott Wisniewski
... only designed for internal use, or for use by at most a hand full of developers and a hand full of applications. In those cases, the cost of following the framework guidelines rarely justifies the inconvience. You have to take those recommendations in context. - Scott Wisniewski
Agreed with Scott. If you are not working on a reusable/public library (API) it's often too much of a hassle. - Pawel Krakowiak
(3) This is nothing specifically to do with generic lists. Non-generic List is no better (obviously worse in most cases), nor is an array. - Daniel Earwicker
(2) Earwicker, if you read the linked page, you can see what the parent poster is talking about. It boils down to this: If you use Collection<T> instead of List<T>, then you reserve the option of overriding the Collection<T>'s add/delete/replace behavior to send out events, do validation, etc. - Neil Whitaker
32
[+19] [2009-03-24 15:46:03] ArielBH

Calling GC.Collect().

It is a rare case when we need to interfere with the Garbage Collector work.


(2) That can make sense in a game. - finnw
33
[+17] [2008-12-22 17:29:30] SpoBo

Not testing!!! This is the biggest mistake I made in the past. Glad I'm finally turning around. Due to testing my code is better, more logical and all of it is being made easier to maintain. Also, I noticed my speed in development went up as well. Not testing is the biggest mistake you can make imo ...


34
[+17] [2009-05-07 21:22:06] Jon Jones

I always get annoyed when I see these in web forms:

try
{
  int result = int.Parse(Request.QueryString["pageid"]);
}
catch
{...

Instead of using

int result;
if (!int.TryParse(Request.QueryString["pageid"] , out result))
{
    throw new ArgumentException.... 
}

I always hated that Web Forms supposes programmers to do this by hand. - Martin Konicek
35
[+17] [2009-05-25 02:13:41] LBushkin

Forgetting that exception catch blocks are matched in the order defined and that more general exceptions will supersede more specific ones.

try
{
  // some code that may throw...
}
catch( Exception x )
{
  // catches all exceptions...
}
catch( NullReferenceException x )
{
  // never reached because catch( Exception x ) is more general...
}

Resharper and FxCop can help avoid this error.


(4) Your example does not compile. The C# compiler actually catches the very error you are describing. There is no need for Resharper or FxCop here. - Timwi
VB.NET also warns about this, but it also has to remove this warning when When is used (irrespective of if it is still valid). - Mark Hurd
36
[+17] [2009-02-26 21:53:54] Beska

I saw this one recently...

public void MyMethod()
{
    if (this == null) 
    {
       // some kind of bizarre error handling...
    }
    ....
}

He insisted that you should always check if what you're trying to look at is null before using it. My claim that you couldn't actually be in this method if "this" was null fell on deaf ears.


I like this answer. Attempting to call MyMethod would throw a NullReferenceException to begin with. - David Anderson
Reminds me of a former colleague who insisted on copying strings in setter methods in case the caller changed the (immutable!) string later. - finnw
Is this really a common programming mistake? - Kyralessa
(3) Dumb is bad. Dumb and thinking you know better - really bad. - Phil
Well, in his defense, if he was used to writing in Delphi, it's understandable; In Delphi (v6, at least), you can call methods on nil-objects. You'd notice your mistake when the method tries to access instance variables. - Lennaert
@Lennaert: That's weird. But, no, this is a C# guy from the bottom up. He's also my superior. Whee! - Beska
I'll second the Delphi bit. If the method is static there will be no dereference of the pointer in order to find the method and thus no error when you call the method. You'll only get the error when you try to access a field. AFAIK that applies across all versions. - Loren Pechtel
Actually, this can happen! (Sort of) - SLaks
I don't believe this is a common mistake. - kirk.burleson
(1) My condolences, Beska. - Rei Miyasaka
Well... I used to think that this==null will never happen but, just like you people, I was WRONG : stackoverflow.com/questions/2464097/… - Andrei Rinea
37
[+17] [2008-12-19 13:40:10] BFree

Raising an event without first checking if it's null: ie:

public event EventHandler SomethingHappened;

private void OnSomethingHappened()
{
   SomethingHappened(this,new EventArgs()); //if no one hooked up to this event, it'll blow up
}

(2) This is C# specific issue, VB.NET RaiseEvent will check it for you. Or is it something else I'm missing? - dr. evil
(10) I'm lazy, so I write: public event EventHandler SomethingHappened = delegate{}; There, now something is always hooked up to it! - Joe
@Joe: And your calls to otherwise unsubscribed event handlers will take 2 times longer than if you'd just checked for null and not default subscribed. Your calls to otherwise single-cast event handlers will take 3 times longer (excluding time within the actual handler, of course). - P Daddy
(4) The performance does not really matter for = delegate{}; (see mafutrct.wordpress.com/2009/07/08/…). IIRC, setting it = null explicitly is impossible at all. - mafutrct
I think you should also copy the event handler reference before test if it is null to avoid problems with race conditions. For a further discussion on this topic check stackoverflow.com/questions/786383/c-events-and-thread-safety question. - Carlos Loth
38
[+15] [2009-05-18 17:19:45] Kyle Gagnet

Forgetting this:

DBNull.Value != null

From MSDN [1],

Do not confuse the notion of a null reference (Nothing in Visual Basic) in an object-oriented programming language with a DBNull object. In an object-oriented programming language, a null reference (Nothing in Visual Basic) means the absence of a reference to an object. DBNull represents an uninitialized variant or nonexistent database column.

The common mistake isn't necessarily thinking that DBNull.Value is a null reference. I think it's more common to forget that your DataTable or other database-sourced object contains DBNulls rather than null references.

[1] http://msdn.microsoft.com/en-us/library/system.dbnull.aspx

+1 I don't forget but I find it a pain to deal with as you end up checking for null to give a DBNull.Value often. - PeteT
39
[+14] [2009-06-23 20:34:33] Mike

I've run into a lot of code that doesn't make use of the TryParse functions in classes (like int.TryParse). It makes the code look a little neater and returns a bool result instead of throwing an exception in which you will have to try to catch.


(2) Just an addition, handling exceptions can be somewhat heavy on the CPU if you expect a lot of errors. - Rei Miyasaka
+1 for TryParse - Andrew Neely
40
[+14] [2009-04-09 22:25:02] user88637

I am a C++ programmer and there was a time when started to code in C# without doing any reading on the language, here are some of the mistakes I did.

Writing a method that write\read your object state to a file instead of using XML (or binary) serializers.

Applying the observer pattern instead of using the built in Events.

Applying the Iterator pattern instead of using enumerators and the IEnumerable interface.

Writing a clone method to classes without implementing the IClonable interface.

Not using the "Using" keyword on IDisposable objects.

Some realy strange things with strings which I can't recall now. (Strings in C# work a bit different then c++)


41
[+13] [2009-02-09 17:11:53] Peter Morris

Your class doesn't need a Finalizer just because it implements IDisposable!

You can implement IDisposable to give your class the ability to call Dispose on any owned composite instances, but a finalizer should only be implemented on a class that directly owns unmanaged resources.

Any compositely owned instances that own unmanaged resources will have their own finalizers.


Good point. That was all I wanted to say, but it didn't have enough characters. - phoog
42
[+13] [2009-07-31 16:37:02] Juri
public DateTime IncrementDateByOneDay(DateTime date)
{
   DateTime result = date;
   result.AddDays(1); //ATTENTION
   return result;
}

This did not change "result". Correct version:

public DateTime IncrementDateByOneDay(DateTime date)
{
   DateTime result = date;
   result = result.AddDays(1);
   return result;
}

This is just an example of course. I found such bugs already inside other methods and it's really hard to find since it seems to be correct by just running over it.


(7) This should have been written as a simple one-liner to begin with: return date.AddDays(1); . Not more needed and much less error prone. - Alex
I think someone posted a similar situation with SubString, definitely something to learn from - SwDevMan81
(2) @Alex, it should not be written at all, if you allready have a datetime-object to send to the function, why not use the .Addays(1) on that object to begin with. (I know, the example was an example to make an point) - Stefan
(1) you know the example is stupid?because it does the same thing as AddDays(1) and it returns a value while the name says it alters the input. - Behrooz
43
[+12] [2009-06-12 03:01:45] SwDevMan81

Failing to synchronize to the GUI thread from a different thread causing an "InvalidOperationException: Cross-Thread Operation not valid" Exception

How to resolve [1]

[1] http://stackoverflow.com/questions/661561/how-to-update-gui-from-another-thread-in-c

(1) This is a good one. - Rei Miyasaka
44
[+12] [2010-03-20 18:27:50] Eric Lloyd

This is one that's probably endemic to any language: Ignoring the rule of "Don't Repeat Yourself", or DRY. I sometimes wish it were possible to administratively disable an IDE's paste functionality beyond a certain clip size.

I've seen (and cleaned up) way too many instances where somebody needed a method "just like this here one", only slightly different. All too often, the method in question is about fifty lines long, and the change they want to make is to some literal magic number that's peppered throughout, so they make a copy of the method, change all the appearances of the magic literal (or whatever trivial change they needed), and call it good.

Sometimes, they don't even commit this travesty at the method level! I once found a pile of cases in a switch where each of the case bodies were so nearly identical (and loooonng!), the "programmer" had to scope each case to avoid renaming his temporary variables. I confronted him about this, and he thought he'd been incredibly clever. Unfortunately, in this particular situation, it would've been impolitic to burst his bubble, but it was tempting nonetheless.

So, people, please, THINK before you insert! (Hmm... That's good advice in life as well as coding, isn't it? ;-) If there's a method that does nearly what you need to accomplish, rework it to additionally support your case and reuse, recycle, rejoice! If it's part of a larger method, extract the code into its own method and have the original method call it! Code may be good, but more code is NOT better!

Ahem. *sigh* I feel much better now. Thank you.


(3) This is one of those things I wish more people would abide by because it would make my life easier when taking over projects. I'm still a fairly new programmer(a little over 2 years as of today) compared to most people, but for some reason the DRY concept came naturally to me before I even knew what it was. - rossisdead
45
[+11] [2011-01-02 23:29:16] Iain

Incomprehensible code, especially branch conditions. I try to document my code with meaningful variable and method names, before I think about using comments.

e.g.

if (x <= 0.3 && penalties.Any(a=>a.Frequency == "week") && st > 0 && st < 10)
{
  ... do something
}

I prefer to extract the branch condition as a variable, giving it meaningful name:

var hasClassOneWeeklyPenalty = x <= 0.3 && penalties.Any(a=>a.Frequency == "week") && st > 0 && st < 10;
if (hasClassOneWeeklyPenalty )
{
  ... do something
}

(1) Why not make it even more documented by creating a function? E.g. NeedToDoSomething? - MarkJ
@MarkJ: I do that too, but not for every condition. I don't think the code would be very readable if I did. Generally I use functions when I want it to be re-usable; it is complex; or its a rainy day :op - Iain
It isn't a major expense but it seems like this would use more memory for a longer period of time as your conditionals remain in scope longer. You could prevent this by wrapping in brackets I suppose. - Chris Moschini
46
[+11] [2009-06-20 19:20:58] zebrabox

Modifying a collection by iterating the collection.

Contrived code sample:

List<int> myItems = new List<int>{20,25,9,14,50};

foreach(int item in myItems)
{
    if (item < 10)
    {
        myItems.Remove(item); 
    }
}

If you run this code you'll get an exception thrown as soon as it loops around for the next item in the collection.

The correct solution is to use a second list to hold the items you want to delete then
iterate that list, that is,

List<int> myItems = new List<int>{20,25,9,14,50};
List<int> toRemove = new List<int>();

foreach(int item in myItems)
{
   if (item < 10)
   {
        toRemove.Add(item);         
   }
}

foreach(int item in toRemove)
{
     myItems.Remove(item);
}

Or if you're using C# 3.0 you can use the List<T>.RemoveAll like this

myInts.RemoveAll(item => (item < 10));

(1) Actually, you can simply write: myInts.RemoveAll(item => item < 10); - ShdNx
And you must mean C# 3.0, since neither C# 3.5 exists nor do lambda expression require .NET 3.5 (in fact, I use list.RemoveAll(x => x < 10) all the time in my .NET 2.0 application, along with all the other C# 3.0 language features, excluding linq). - Allon Guralnek
@ShdNx - Thanks for the tip :) @Allon - Yep, thanks for the correction, I'll update - zebrabox
how about: for (int i = 0; i < myItems.Count; ) { if (myItems[i] < 10) { myItems.RemoveAt(i); } else { i++; } } - SwDevMan81
You have to use a reverse for loop, if you remove item 5, then item 6 becomes items 5, but you have already moved to index 6. If you use a normal up counting loop you skip items. - Anthony D
@Anthony D - Thats why the i++ is in the else statement. - SwDevMan81
Another option is to call ToArray() on it: foreach (int item in myItems.ToArray()) { if (item < 10) { myItems.Remove(item); } } - SwDevMan81
47
[+11] [2009-04-27 07:34:55] Amby

Not using flagged enum, where actually we need to.

 public enum Hobby {
    None,
    Reading,
    Cooking,
    Cricket,
    All   
    }

 public class Student {
      public List<Hobbies> Hobbies{get;set;} 
//Bad: we have to make it a collection, since one student may have more than one hobby.
    }

Better Idea.

[Flag]
public enum Hobby {
None=0,
Reading=1,
Cooking=2,
Cricket=4,
All=7   
}
public class Student {
  public Hobby Hobby {get;set;}; 
//Now, we can assign multiple values to it, eg: student.Hobby= Hobby.Reading | Hobby.Cooking;
}

For more info: http://msdn.microsoft.com/en-us/library/cc138362.aspx


Also check out using this kind of extension: stackoverflow.com/questions/7244 - Keith
While this is good for cases where you have A, B, C or All, this is complete nonsense for generic lists like a list of hobbies. In the first example, the "All" doesn't make sense at all - it is not a hobby! So, while the point might be right, your example is horribly confusing and/or wrong. - Sander
@Sander, Amby's point (perhaps with not the best example, but most examples are imperfect) that some people design an enumeration intending them to be treated as flags, but not marking them with that attribute. Contrary to Amby's point, though, it IS still possible to use such an enumeration as the basis for testing a field, but you always have to cast it. - Nij
... to be more specific, your member variable can not be typed to the enumeration, but the type underlying the enumeration (default is int). The result is horrid casts: - Nij
why use flags? I don't see why it matters. Aren't flags outdated? - Alex Baranosky
Yuck. I'd sooner use a collection that you can add Hobby objects to than an Enum for this. Granted, you could erroneously try to add a Hobby more than once to such a collection; but you can use a Flag enum incorrectly as well. I believe the Microsoft guidelines discourage flag enums these days. - Kyralessa
@Kyralessa: You'd really rather create an object and a container when all you need is two ints? Then presumably loop through your container to see if a flag is set? It's not pretty but it is efficient. - JonB
48
[+11] [2009-02-25 09:16:29] Enrico Campidoglio

In many cases I've seen this scenario:

IDbConnection conn = null;

try
{
    conn = new SqlConnection("SomeConnString");
    conn.Open();
}
finally
{
    // What if the constructor failed?
    conn.Close();
}

If an exception originated in the constructor of SqlConnection class, the variable would never be assigned, which will cause a System.NullReferenceException in the finally in the Close method never being called, resulting in a memory leak.

Generally, it is always a good idea to be as defensive as possible in the catch and finally blocks, since that code is critical and simply can't fail:

IDbConnection conn = null;

try
{
    conn = new SqlConnection("SomeConnString");
    conn.Open();
}
finally
{
    if (conn != null)
    {
        conn.Close();
    }
}

(21) Better still, use a Using statement to ensure the connection is both closed and disposed. - Dan Diplo
(1) +1 for paranoia. It's saved me more than once. - Andrew Neely
49
[+10] [2009-02-08 21:37:55] Peter Morris

Adding threads to an app without knowing the basics of writing threaded apps.


Not going to lie, I am guilty of this, but so far it has worked well. Now using Tasks and Funcs I almost never run into a problem, even when I whip up blocking collections to parallelize my data processing. - wllmsaccnt
50
[+10] [2008-12-19 12:35:54] Charles Graham
  • Using View State instead of Control State. View State saves everything, where as control state saves only the information needed to operate the statefull control on the statelessweb enviernment.
  • Not using paterns such as MVP in order to reduce coupling.
  • Having too much logic in the view instead of the domain objects.

51
[+10] [2008-12-21 00:41:39] Soviut
  • Serializing huge amounts of data to the Viewstate in ASP.NET. Things like storing a serialized User object in the Viewstate rather than just using a session properly.
  • Including an entire, massive, Viewstate crushing toolkit just to add an "are you sure?" prompt or a tooltip.
  • Being afraid to use lightweight toolkits that don't specifically target ASP.NET, such as JQuery or Prototype. (note: JQuery is now being adopted by Microsoft as the premiere client side toolkit for ASP.NET MVC).

52
[+10] [2011-01-02 20:15:42] Daniel Peñalba

In System.Windows.Forms, it's a common problem filling a ListView or a TreeView without BeginUpdate()/EndUpdate(). It causes a blank window when you fill the control in the main thread.

The correct way to do this is the following (also using AddRange() instead iterate and add one by one):

public void FillList(ListView view, IEnumerable<string> items)
{
    //if you don't use BeginUpdate
    //you can cause a blank form 
    //if the list is big
    view.BeginUpdate();
    try
    {
        //Single call, minimal overhead, 
        //and only one potential reallocation
        view.Items.AddRange(items);
    }
    finally
    {
        //remember doing this in 
        //a finally statement
        view.EndUpdate();
    }
}

(3) initialization should be done before the try; cleanup inside the finally. BTW: This pattern would be a good candidate for an example of an extension method, or a generic method. - Jeroen Wiert Pluimers
Yes, good point, thanks for the update. - Daniel Peñalba
53
[+10] [2010-10-14 11:36:45] Rei Miyasaka

Returning null when an empty list/sequence/array would suffice.

HtmlAgilityPack [1]'s SelectNode function returns null instead of an empty list, so what I could have written simply as:

var links =
    node.SelectNodes("//a")
    .Select(n => n.Attributes["href"].Value);

Ends up having to be:

var linkNodes = node.SelectNodes("//a");
IEnumerable<string> links;
if (linkNodes != null)
    links = linkNodes.Select(n.Attributes["href"].Value);
else
    links = new string[] { };

Urrrghhhh!!!!1

[1] http://htmlagilitypack.codeplex.com/

I agree. "Framework Design Guidelines" book recommends returning empty lists/arrays in cases such as this. - Arnold Zokas
(1) +1 If you are feeling pompous, you can call this an example of the null object pattern - MarkJ
@MarkJ Cool, I'm pompous and like new words :) - Rei Miyasaka
54
[+9] [2010-10-20 17:44:36] Hooch

Never make your Windows Forms controls public. Always keep them private and use properties if you need to.

    public string Name
    {
        get
        {
            return textBoxName.Text;
        }
    }

and when you want to parse some text from Other form make as much as possible in that form to keep main form clear.

    public string Command
    {
        get
        {
            return "#Wait(" + textBoxTime.Text + ")" + Environment.NewLine;
        }
    }

(2) +1 I've seen this so many times. Someone has form A that calls form B and someone else comes along and calls from B from form C but changes form B in form C. B is almost what they what they want, but they need to move some buttons. Someone else comes along and has to modify form B. They are almost guaranteed to break something because they did not know about form C. I call these time bombs because they will blow up eventually it's just a matter of when. PLEASE use properties or pass something on the constructor. - dwidel
+1 for encapsulation! Trying to refactor programs which break encapsulation is a sure way to develop an ulcer. - Andrew Neely
55
[+9] [2010-08-18 20:16:01] Mahol25

Not understanding memory managment and garbage collection

The pit fall is assuming that just because it is a managed environment, I don't have to know about memory management. That's a mistake. Not learning about these things will eventually lead to memory leaks.

This includes correct implementation of IDisposable and how to manage disposable objects correctly.


Also, detaching event handlers appropriately - I've seen plenty of code that causes memory leaks by leaving event handlers attached, particularly in UI code - Paul Nearney
I would say event handlers are the big key here, we have a large .net server app that leaked very badly due to poor IDisposable implementation and improper event handling. - Justin
56
[+9] [2010-09-18 20:49:10] Rei Miyasaka

Believing that enums are exhaustive:

enum YesNo
{
    Yes,
    No
}

string GetColloquial(YesNo value)
{
    switch (value)
    {
        case YesNo.Yes:
            return "Yeah";
        case YesNo.No:
            return "Nope";
        default:
            return "This will never happen"; //oh yes it will!
    }
}

Enums are internally implemented as integers, so you can still do things like this:

Console.WriteLine(GetColloquial((YesNo)5));

This one can be dangerous, so watch out!


Huh... didn't know that, but I believe you - what I think SHOULD happen is an InvalidCastException if the value doesn't map to any Enum value. Microsoft: get on that! - Pwninstein
(1) Agree, though in MS' defense, this is a surprisingly complicated problem. It's important when the enum is a set of flags rather than just a mutually exclusive selection, because then the value needs to actually be calculated with binary arithmetic. Versioning issues makes it even more difficult to just make it really strict. One solution might be to throw a compiler error if default isn't handled by a switch clause. F# does this, but it wouldn't be as effective in C# because it's more common in C# to use complex execution paths than in F#, which relies primarily on switches (matches)... - Rei Miyasaka
57
[+9] [2008-12-19 20:32:54] Michael Stum

Setting local objects to null will not free up any resources [1], so there is usually no reason to do it.

[1] http://stackoverflow.com/questions/2785/

(4) This is usually good advice for locals, but often bad advice for members. It often makes logical sense to assign null to a member to indicate that it is restored to its initial state. - Daniel Earwicker
(1) I agree with Earwicker. Setting objects to null can cause the garbage collector to collect them and free up the used memory resource. I think you've taken that question you've linked slightly out of context and over-generalized it. This answer only applies in some circumstances. - Scott Langham
I never thought about that, but now that you mention it - you're right, this only applies to locals, because they will go out of scope rather fast, whereas members can stay in scope for a long time. Modified article, thanks for the comments! - Michael Stum
(1) For members no, but for local variables yes, it drives me mad! I read that actually it postpones collection because setting the variable to null accesses the variable which implies to the GC that the value it holds is still required. - Peter Morris
This drives me crazy also. The project I'm working on now at the end of every function has a list of local variables they set to nothing. I've asked why, and the answer was, well this one time it fixed a memory leak so now we set all variables to nothing. I want to say no, it didn't, but... - dwidel
@Peter Morris Is this from the C# FAQ what you read? - MarkJ
58
[+9] [2008-12-19 23:08:52] Tim Merrifield

Unnecessary boxing and unboxing.


Could you give an example? - Anthony
Yeah, that would be good. - Andrei Rinea
(2) Sure, one example would be using an ArrayList to store a collection of value types. So if I use an ArrayList to store ints. When I add or remove an integer it must be boxed/unboxed. Clearly this is wasteful. Maybe a bad example but you get the idea. - Tim Merrifield
Boxing is bad... mkay... - BigBlondeViking
59
[+9] [2009-06-25 11:27:29] Bluestone

Similar to this answer [1], but using String.Format.. It helps readability a lot..

Instead of this..

Console.WriteLine("A: " + 9 + "\nB: " + 34 + "\nC: " + someInt);

Use..

Console.WriteLine("A: {0}\nB: {1}\nC: {2}", 9, 34, someInt);
[1] http://stackoverflow.com/questions/380819/common-programming-mistakes-for-net-developers-to-avoid/381220#381220

Please note that you don't need String.Format() to reach this effect as Console.WriteLine has the same behavior if I am not mistaken. - Tom Wijsman
(1) @Tom: True, the Console.WriteLine has an overload that saves you a call to String.Format. I just wish Debug.WriteLine has such an overload as well... - Allon Guralnek
I took the liberty to correct it. - Konrad Rudolph
(5) I just wished that the {0}...{x} parameters could be optional named for some clarification when working with multilanguage-strings and translations. Instead of string.format("You are {0} years old and we have {1} people who are at same age. The median age are {2}",x,y,z) this would be prefered in some cases when sending strings to translators: string.format("You are {0:Age} years old and we have {1:AmountOfSameAge} people who are at same age. The median age are {2:MedianAge}",x,y,z) - Stefan
60
[+8] [2008-12-20 23:24:45] petr k.

One common mistake is using

if (obj is SomeType) {
  ((SomeType)obj).SomeMethod();
}

instead of

SomeType tempVar = obj as SomeType;
if (tempVar != null) {
  tempVar.SomeMethod();
}

The latter does not double the cast (which does occur in the first snippet and means a slight perfomance hit).


(1) The corollary to this is SomeType tempVar = obj as SomeType; tempVar.CallMethodAssumingNotNull(); - Cameron MacFarland
(1) Without profiling data indicating that this micro-optimization will improve my user's experience, I would choose the one that more clearly indicated my intent in code. - Jay Bazuzi
I timed it and didn't notice any difference. So I prefer the first one. It doesn't clutter the function with a new variable that some other programmer could accidentally try and access later in the function without realising it may be null because it wasn't of SomeType. - Scott Langham
(15) Downvoted: In a loop of 10 million times it was 10 milliseconds slower. I'd rather go for the 2 lines of code that don't need a local variable. - Peter Morris
+1 the right way to do casting in c# - Martin Clarke
(1) Downvoted because this is a prime example of premature optimisation. - Timwi
(2) Upvoted because the latter is thread-safe. - Paolo Moretti
(notvoted) I would assume the compiler would handle these optimistions for me - Oxinabox
61
[+8] [2009-08-18 20:31:17] Carlo

Risky code in the constructor. When I have to initialize an object with risky code (for example, loading from XML), I usually have an Initialize() or Load() method, so I don't do it in the constructor, which can leave the object in an "incomplete" state. So instead of

try
{
    MyObject o = new MyObject();
}
catch(Exception ex)
{
    // Handle exception.
}

I do this:

MyObject o = new MyObject();

try
{
    o.Initialize();
}
catch(Exception ex)
{
    // Handle exception.
}

62
[+7] [2010-02-28 00:42:02] Thomas

The common coding mistake I find the amongst by people inexperienced with C# is that they do not account for the fact that string comparison is case-senstive (unless you explicitly specify otherwise) especially when the comparison value comes from a database:

var foo = dataRow["ColumnName"].ToString();

if (foo = "ABCDEF")
    ...

The most common reaction change I see is to convert both values to uppercase or lowercase instead of using the Equals method with a StringComparison enum like so:

//bad
if ( foo.ToUpper() == "ABCDEF")

//correct
if ( "ABCDEF".Equals(foo, StringComparison.CurrentCultureIgnoreCase )
    ...

( OrdinalIgnoreCase or InvariantCultureIgnoreCase...)

Obviously, there is also the problem with using string literals, but that has already been mentioned.

The most common mistake I see with people that write code that interacts with a database is not accounting for nulls.


Yeah, but burying code with international considerations when the application is used by 30 monolingual users in one office is silly. The "correct" way is almost 3 times the typing and visually cluttered for reading. - MatthewMartin
(1) @MatthewMartin - Once you realize that == is case-sensitive, explicitly declaring the case comparison to easier to read IMO. If I see foo == myspiffyvar, I have to stop and think about whether the original developer understood that the comparison was case-sensitive or whether they overlooked it. That requires more effort on the part of the reader. Further, the extra typing isn't an issue with intellisense and autocomplete. - Thomas
I always trim spaces and cast to upper before comparing user input against a known value. To people, "dog"="Dog" =" DOG" it still barks. - Andrew Neely
63
[+7] [2010-06-24 02:06:00] Martin Ongtangco

in C#, a lot of people are still using the "" to declare an empty string. I consider this as a bad programming practice. people should use string.Empty to strictly specify that the string is empty and not a empty quote that looks like somebody accidentally deleted the string.

example: BAD

string someString = "";

example: GOOD

string someString = string.Empty;

(11) You're discussing coding standards here, not real mistakes that break stuff. - vdboor
Well, it breaks my heart to see this kind of programming... common errors come from wrong coding procedures that lead to communication issues among working developers. - Martin Ongtangco
(3) In that case I still think a different example is needed, and you're overreacting a little for this case. The compiler can easily optimize "" out for string.Empty. - vdboor
(3) .Net needs a HeartBreakException. - rossisdead
(2) I think it makes the code more complicated to write if its just a "known variable type" such as string. Just wondering if you also initialise your int like this: int number = int.MinValue; - BerggreenDK
64
[+7] [2010-07-28 23:57:42] Marko

Overengineering [1]...

Starting with interfaces and models and concepts and then getting stuck because I didn't notice this and that, then changing interfaces and classes and change code and then really getting stuck...

I've learned: build systems from a ground up, not the opposite :)

[1] http://en.wikipedia.org/wiki/Overengineering

This seems like a general anti pattern to avoid, and not something specifically related to .NET developers. - Jan Aagaard
Yes, I agree... although it could be applied to .net and I learned it doing .net technologies... - Marko
65
[+7] [2010-11-17 06:58:03] Zain Shaikh

Avoid using .ToString() on a string.

I saw the following code a few days ago.

string keyName = @"HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows\CurrentVersion\Run";
Microsoft.Win32.Registry.SetValue(keyName, "MyStartUp", Application.ExecutablePath.ToString());

Where Application.ExecutablePath is already a string. So there is no point in using .ToString().

One more example I saw:

Request.QueryString["search"].ToString()

Request.QueryString returns a string, so one should not call .ToString() here.


Would anyone really make this error? - Robert Harvey
yes of course, people do, see the one more example I haved added in my answer. - Zain Shaikh
(1) I dont know why anyone would do like this, but I have see developers doing so and that is why I posted it over here. - Zain Shaikh
(3) I think this is more likely to be a result of simple absent mindedness than a bad habit. Anyway, it's not like it really causes problems; it's merely tautological. - Will Vousden
66
[+7] [2010-10-18 19:26:56] townsean

This one almost bit me good:

Prefer readonly to const. [1]

private const string connectionString 
     = Properties.Settings.Default.ConnectionString;

Since the const connectionString is resolved at compile time not runtime, I mistakenly believed that this was actually configurable.

What I needed was a static readonly variable as such:

private static readonly string connectionString 
     = Properties.Settings.Default.ConnectionString;

Which is instead, resolved at runtime. :)

I link Effective C# because I had read an item addressing why you should use readonly over const like a week before so it caused me to realize the error sooner. :)

[1] http://rads.stackoverflow.com/amzn/click/0321658701

(1) You missed the static keyword, which omitting it also costs performance cuz it's being created for each instance... - Shimmy
I've updated your post. - Shimmy
67
[+7] [2009-06-20 18:55:03] HomieG

Check out the book Framework Design Guidelines [1].

It contains a lot of DOs and DO NOTs.

[1] http://rads.stackoverflow.com/amzn/click/0321545613

I hope it says "do not design frameworks" - finnw
I wish every .NET developer would read that book. - TrueWill
68
[+6] [2008-12-22 22:42:13] Dmitri Nesteruk
Assert.AreEqual(0.00, resultOfCalculation);

Comparisons with 0.00 aren't a good way to go, especially in engineering - in a complex calculation, the end result might diverge by at least double.Epsilon and typically a lot more due to all sorts of precision loss.

This is why some test frameworks overload the AreEqual() method, allowing the user to specify tolerance values:

Assert.AreEqual(expectedResult, actualResult, tolerance);

Down voted because (1) You are so vague that I have no idea what you are trying to say and (2) because the parameters are the wrong way around. If you fix your answer nudge me and I will consider removing my down vote. - Peter Morris
Fair enough. Fixed. - Dmitri Nesteruk
(2) There's a Assert.AreEqual for floating point numbers, that take a tolerance. Use that if you must compare non integers in a test. - Brian Rasmussen
(5) 0 is an integer. It is reasonable to assert that an integer variable is zero. Did you mean 0.0? - finnw
69
[+6] [2008-12-27 22:29:47] community_owned

Not quite a language thing, and perhaps arguably a "mistake" but...Deploying web-based apps without using HTTP Compression (GZip, Deflate, etc.) There rarely a good reason why you should not use HTTP Compression...

Also, not checking Page.IsValid() to ensure server side validation is done.


70
[+6] [2008-12-19 20:27:56] DD59
 i = i++;

is not the same as

 i++;

I found a great Stack Overflow post about this, but this also does a decent job [1].

It's a dumb thing to do anyway, but the point is that it increments differently from C++.

Ah, here is the Stack Overflow one, What’s the difference between X = X++; vs X++;? [2].

[1] http://weblogs.asp.net/kennykerr/archive/2006/03/29/Increment-differences-in-C_2B002B00_-and-C_2300_.aspx
[2] http://stackoverflow.com/questions/226002/whats-the-difference-between-x-x-vs-x#226019

(3) i++ is more like i = ++i (increment before assign) - jishi
71
[+6] [2010-10-14 22:29:04] Rei Miyasaka

Believing that casting a collection to IEnumerable makes it read-only.

GetReadOnlyList does not return a read-only collection. It can be written to by casting back to List:

public IEnumerable<int> GetReadOnlyList()
{
    var list = new List<int> { 1, 2, 3 };
    return list as IEnumerable<int>;
}

public void UhOh()
{
    var list = GetReadOnlyList() as List<int>;

    list.Add(4);
}

You can avoid this is by returning list.AsReadOnly() instead.


Upcasting is terrible practice in general. There are MANY types, including any custom types that could implement IEnumerable, so just upcasting it to a list is asking for Exceptions. If someone hands you an IEnumerable, you treat it as an IEnumerable. - DevinB
Aye, but someone who tries to do this is often doing it for malicious purposes anyway. For instance, you might expose plugin APIs, thinking you're safe because you're returning immutable collections probably with immutable content, and then someone might come along and remove all the entries... Or someone might edit the return value thinking it's a clever way to improve performance. - Rei Miyasaka
72
[+6] [2010-07-29 16:44:58] xport

Misconception about static fields.

If two instances of an assembly running on two separate processes, the static fields are no longer related. Modification on static fields in the first process will not affect static fields in the other process.

My pitfall when I learnt C#, I get provocated with the statement that "all instances of the class will share the same static fields". This statement does not emphasize that it is only true if all instances are in the same process.


(2) even more! They don't need to be in separated processes but even only in separated AppDomains and the static stuff will not be shared. - Andrei Rinea
73
[+6] [2009-12-08 00:20:28] anirudhgarg

Making sure to understand that strings are immutable in .NET.

So, whenever you do any operation on a string you get back a new string with the operation applied to the new string.

So if you have a lot of operations in a loop ( say appending values to a string ), it is instead recommended to use the StringBuilder object.


74
[+6] [2009-09-04 08:19:04] Ami

Well, I went through all the answers, and they're all very important things to look out for, but I think the number one mistake of programmer is not documenting their code!!

It's not really a pure coding issue, but nevertheless I believe it is a most basic skill.

Everyone makes mistakes, all of us write bugs. That's all human. But not commenting on your code, thereby making life easier on yourself, and more importantly for your fellow developer that will need to debug your code after you moved on, is the worst thing IMHO.

Also, not using common design patterns (everyone knows singleton, but strategy, adapter, decorator, etc...), or not even knowing them, is something I see quite often.


(1) I've heard of "singleton" but I have no idea about the other patterns. Does that make me a worse programmer? I don't think so. I bet I probably already use those patters in some appropriate circumstances, I just don't recognise them as patterns and I don't know their names. I don't feel that I have to. - Timwi
(3) IMHO documenting code is not as important as writng self documented code... - moi_meme
75
[+5] [2011-01-02 19:53:41] dwidel

My pet peeve is when people just mindlessly put try catch statements on everything, print the message and go on their way. Consider this example that is not real different from something I once saw in production.

   Dim SQL As String = "Delete from Orders "
    SQL &= GetWhereClause()




Private Function GetWhereClause() As String
    Dim rv As String = ""
    Try
        'lots of code here.
    Catch ex As Exception
        MsgBox(ex.ToString)
    End Try
    Return rv
End Function

Yes, the program doesn't crash, but it will trash their database! Now, obviously that's not the only thing wrong with this code. But it's an obvious example of many situations that are a lot more subtle.

The main purpose of a try catch is flow control, not to make sure your program doesn't crash. They'd have been better off crashing than catching the error. So take a minute and think about what state you are in if you catch an error, what can you do to fix the state, and are you in the correct place in the program to take this action. If not maybe you need to put the try catch somewhere else.


76
[+5] [2011-02-20 14:46:48] Thomas

Not checking input parameters of functions (and not checking output values of functions)

BAD

public void DoSomething(Bar foo, X x)
{
    foo.DoItOnObject();
    x.DoItOnObject();
}

Good

public void DoSomething(Bar foo, X x)
{
    if (foo == null)
    {
         throw new ArgumentNullException("foo");
    }
    if (x == null)
    {
         throw new ArgumentNullException("x");
    }        
    foo.DoItOnObject();
    x.DoItOnObject();
}

Why: Which of the objects foo or x was unassigned before calling the method when you receive a NullReferenceException in this function. In release code: No chance.

Thinking that you cannot program memory leaks in .Net.

Example

public class X
{
    public void DoSomething(object sender, EventArgs ea) { }
}

public class Y
{
    public event EventHandler<EventArgs> MyEvent;
    public DoSomething(X x)
    {
         this.MyEvent += x.DoSomething;
    }
}

Y y = new Y();
for (int idx = 0; idx < 10000;idx++) {
    X x = new X();
    y.DoSomething(x);
}

and afterwards believe that the 10.000 objects are not in memory.


There really needs to be language/platform-level support for weak events. - Rei Miyasaka
77
[+5] [2009-02-26 21:31:00] Gregory A Beamer

I would say mine are more general, as I deal with good architecture more than coding nick nacks:

  1. Applications are logic, not UI. You are building an insurance application, not an ASP.NET application.
  2. Putting anything other than UI logic in the UI layer. This makes for very inflexible applications that have to remain a certain type.
  3. 0% Code Coverage with tests. Big pet peeve. I know development is an art and a science but that means you have to have at least SOME science.
  4. Belief that small apps do not require design. Small apps routinely become large apps.

String concatenation, already mentioned, is a big no no for me. Another is try ... catch. If you are not catching, use try finally. Just as a hands up for noobs, these are functionally equivalent:

using(SqlConnection conn = new SqlConnection(connString))
{
    conn.Open();
}

AND

try
{
   conn.Open();
}
finally
{
    conn.Dispose();
}

One big one I have seen over and over again, with ASP.NET, is writing ASP.NET like ASP. The same can be said for VB.NET devs who moved over from VB. Binding = good; looping to Response stream = bad.

What else>? Oh, you can run through almost any sample on the web and see some pretty bad examples of proper architecture. I do not denigrate the writers, as they are not there to illustrate architecture, but to show how to solve one problem, but still ...


The .Net 3.5 compiler has got really good at fixing string concats - take a look at the IL after it has been compiled with optimisations on. - Keith
78
[+5] [2009-06-14 21:02:37] Jacob

This one sometimes gets us: in LINQ [1], using First() instead of FirstOrDefault() on a IEnumerable that may be empty. Sometimes, it seems intuitive that First would return a null, rather than what it does with the exception throwing.

[1] http://en.wikipedia.org/wiki/Language_Integrated_Query

While we are around here. Using First(cond) when we mean Single(cond). (single throws an exception if there is more than one thing that meets that condition. - Oxinabox
79
[+5] [2009-05-25 02:07:30] LBushkin

Not writing defensive code that checks for nulls.

MyClass t = GetAnInstance();
t.SomeMetehodCall(); // may throw NullReferenceEx

// or worse...

SomeOtherFunction( t ); // may also throw NullReferenceEx

Debugging NullReferenceExceptions can be a frustrating time-killer in .NET development.


(6) Depends, if GetAnInstance will ever return null? If not, don't check it. If for some odd reason it does when it never should, you now know that GetAnInstance is wrong. - Sekhat
80
[+5] [2009-07-31 16:41:58] Juri

Similar to this one [1], when defining a property

...
private string _MyString;
public string MyString
{
  get
  {
    return MyString; //ATTENTION
  }

  set
  {
    ...
  }
}

At runtime this gives a nice crash. Of course this should be written as

private string _MyString;
public string MyString
{
  get
  {
    return _MyString;
  }

  set
  {
    ...
  }
}
[1] http://stackoverflow.com/questions/380819/common-programming-mistakes-for-net-developers-to-avoid/1164818#1164818

You can generate the properties automatically, the Visual Studio takes care of it and nothing could be wrong... - Tomas Walek
(2) Of couse, but often there is the need to still keep a private variable instead of the automatic property declaration :) - Juri
(1) @Thomasek : Not Visual Studio, the compiler takes care of it. - Andrei Rinea
Hah, this messed me up a few times. - Rei Miyasaka
81
[+4] [2009-07-22 12:05:47] Dan Diplo

One horrible pitfall is a method calling itself when you meant to call an overload eg.

	public static void MyMethod(int id, bool always, string filter)
	{
		// Do something
	}

	public static void MyMethod(int id, bool always)
	{
		MyMethod(id, always); // Ouch!
	}

	public static void MyMethod(int id)
	{
		MyMethod(id, false, null);
	}

(1) Viva optional parameters, no need to write these useless overloads - moi_meme
(1) @moi_meme: If it's a public method in a public class, you don't really want to use optional parameters as they are compiled into the caller. Great fun when you change the Default Value of one of them and wonder why it doesn't work :) stum.de/2010/04/19/… - Michael Stum
@moi_meme: Perhaps a better example whould be on which isn';t jsut a case of argument forwarding, (eg if a refercne type had to be constucted, or a Factory accessed) - Oxinabox
82
[+4] [2008-12-19 23:23:16] Patrik Hägne

As some have mentioned swallowing exceptions is a really bad habit. Another thing that I see people do all the time that's quite similar to swallowing exceptions is redundant null checks.

Let's say that we have a method that returns an object and we it should never return null a lot of times you'll see this.

var o = this.Foo();
if (o != null)
{
  o.Bar();
}

// more code here...

Really if the method Foo returns null this is exceptional behavior and an exception should be thrown, so either in an else block throw an appropriate exception or just remove the if-statement and allow for the NullReferenceException to be thrown.


(1) This is debatable. I think this is just another case of using exception handling for control flow. The only time I might agree is if this method is certain to not return null under normal circumstances. But, I also don't like using var in this case when the type returned is non-obvious to the reader - BobbyShaftoe
First of all, as I say this is when the method never should return null, in other words if null is returned it's an exceptional behavior. Using var in this case is because it's pseudo code. - Patrik Hägne
(1) I don't know that allowing an NRE is a good option. Better to throw an ArgumentNullException as soon as you know something's wrong. - Jason Baker
The NRE is way better than the alternative specified in the example which is the equivalent of swallowing a checked exception. Of course the called method shouldn't return null, so the method should've probably thrown an exception. I'm not sure I explained this one well enough. - Patrik Hägne
NRE are suggested to be reserved for certain Library classes only: msdn.microsoft.com/en-us/library/… But that isn;t the point of your post - Oxinabox
You should never throw the NRE. You should just not prevent it from being thrown in this case. - Patrik Hägne
fair enough. I see the argument for for catching and rethrwoing NRE as ANE. Wuld you say this is an Antipattern? - Oxinabox
83
[+4] [2010-09-18 15:48:49] Danny Chen

Here is one in ASP.NET.

<form id="form1" runat="server">
    <asp:placeholder id="ph" runat="server">
        <asp:TextBox id="tb1" ruant="server" Text="test" />
    </asp:placeholder>
    <asp:TextBox id="tb2" runat="server" Text="another" />
</form>

And in the code behind it tries to do something to all TextBoxes.

foreach(TextBox tb in form1.Controls.OfType<TextBox>())
{
   //Here it can only get tb2
}

because .Controls returns the first-level child controls only (in this example will return ph and tb2. To get all child controls, you need a recursion:

public static IEnumerable<T> GetChildControls<T>(this Control control) where T : Control
{
    var children = control.Controls.OfType<T>();
    return children.SelectMany( c => GetChildControls<T>(c)).Concat(children);
}

84
[+4] [2010-10-13 23:14:09] Andrei Rinea

Avoid doing risky instantiation in a static field initializer

WRONG:

public class Whatever {
     private static SomeExternalComObject externalObj = new SomeExternalComObject();
// ...
}

Slightly better:

public class Whatever {
     private static SomeExternalComObject externalObj;

     static Whatever() {
        try {
            externalObj = new SomeExternalComObject();
        } catch(ComException ex) {
            // backup strategy and/or logging and/or whatever else
        }
     }
 }

Otherwise debugging will be a great pain in the ... neck.


(1) Using the static constructor is a violation according to the Code Analysis in Visual Studio 2010. msdn.microsoft.com/en-us/library/ms182275.aspx It is recommended to use an inline call to a static method instead. - Pierre-Alain Vigeant
(1) Yes, that is why I called the second block "slightly better" not "CORRECT" or "Good" or sth.. - Andrei Rinea
85
[+4] [2010-10-14 07:52:15] xport

Array of non-object cannot be cast to array of object

        int[] data = { 1, 2, 3 };

        object[] objs = data;//compile time error!!!

That's because int is a value type. If you had a reference type array, it can be converted to an object array. This actually bit me when I used a method like method(params object[] objs) and passed it a single string array (which was implicitly converted to an object[]--I intended for it to be an object[] with a single object--the string[]). - Matt Smith
@Matt: I almost forget my post here. :-) - xport
86
[+4] [2009-08-24 08:17:37] Dror Helper

Not using Events, as a developer that came across from C++ (many years ago) I found that I programmed C++ in .NET instead of using the built in functionality.

In on of my first .NET project I've implemented the observer pattern using interface and inheritance instead of simply using events.


(1) Hmmm, I think you've stated this a little too generally - interfaces can be preferable where you want to require the client implement a set of methods as part of the contract between your component and the user of the component. One of the reasons events are typically preferable for generation of events is that the user can select individual events to optionally subscribe to. There may be cases though where you want to require that the user handles a complete set of events, in which case an interface would be the way to go. - Phil
#Phil of course you're right - there are times interfaces are the right choice for implemeting the observer pattern. In my case it wasn't ;) - Dror Helper
@Phil "a complete set of events" can also be represented by a single event (the event args may need to supply additional information to discriminate between various sub events)...hence IMO the observer pattern is best implemented using events. Also FYI interfaces can contain events too. - SDX2000
87
[+4] [2011-04-02 13:02:39] Eskat0n

Don't utilize all features of .NET platform. Just a little example:

string.Format("({0})", doubleValue.ToString("0.##"))

instead of:

string.Format("({0:0.##})", doubleValue)

Don't using monads.

value = obj == null ? null : obj.Value

instead of:

value = obj.With(x=>x.Value)

One of applicants for position in our company write such code in his test task:

if (value.GetType().ToString() == "System.Double")
{
 // ...
}

Of course we dumped him.


88
[+3] [2011-08-11 11:31:16] Nighil
        DataTable dt = "Data from database";
        gridView1.DataSource = dt;
        gridView2.DataSource = dt; 

when ever you change data in gridview1 then gridview2 will be also effected, instead use this

            DataTable dt = "Data from database";
            gridView1.DataSource = dt;
            gridView2.DataSource = dt.Copy(); 

89
[+3] [2009-12-08 00:00:40] SwDevMan81

Handling ThreadAbortException:

try
{
}
catch(ThreadAbortException ex)
{
}

It's hard (if not impossible) to write code that will always handle a ThreadAbortException gracefully. The exception can occur in the middle of whatever the thread happens to be doing, so some situations can be hard to handle.

For example, the exception can occur after you have created a FileStream object, but before the reference is assigned to a variable. That means that you have an object that should be disposed, but the only reference to it is lost on the stack somewhere...

An addition side note to catching the ThreadAbortException is when you call .Abort() on a thread, it throws an uncatchable ThreadAbortException. "Uncatchable" because even if you do catch it, it is automatically rethrown at the end of the 'catch' block, so you can't stop its propagation. This does, then, have the effect of terminating the thread (provided it doesn't go into a loop in the 'catch' clause).

Finding an alternative is going to save you a lot of time and headache's down the line.


I don't understand what you are saying. You say your exception could happen in the middle of creating a FileStream object, and therefore you shouldn't catch the exception? That surely doesn't follow. - Timwi
@Timwi - No, what I'm saying is that the asynchronous nature of calling Abort can cause problems within your code (the example being a FileStream object is created, but not assigned to a variable). - SwDevMan81
Yes, the issue here is you should avoid using Abort. Nevertheless, it is a useful method to catch Ctrl+C in shell-like applications. - Mark Hurd
Aborts are catchable. Call Thread.ResetAbort(); - Groxx
90
[+3] [2010-01-09 16:03:31] Lawrence

Use background workers to make sure your Windows Forms application does not freeze while executing a thread. End of.


91
[+3] [2010-01-09 16:18:03] Shimmy
string foo = GetString();
if (foo == null) throw ex1;
if (string.IsNullOrEmpty(foo)) throw ex2;

foo is checked twice for null.


92
[+3] [2010-07-29 16:54:47] James Campbell

Worst programming mistake that bothers me daily, especially in others code is not commenting on your code. Properly commented code is your friend.


(17) Comments are the enemy. Rarely kept in sync with code; often point to unclear code or where a method can be extracted. Only add comments in exceptional circumstances. If your code needs a comment it is often the code that is the mistake. - John Nolan
Take a listen to this podcast. it's called : Why Comments Are Evil and Pair Programming With Corey Haines -deepfriedbytes.com/podcast/… - Morten Anderson
(3) There was a great response in another question, I can't remember which one, "A comment is an apology". - Andrew Barrett
(2) Where i work I deal with a crapload of others peoples code, not small projects either. If the programmer had at least made some comments here and there, it would have made my job alot easier, and after years of doing this, it has been a pain without knowing what they were thinking or what also may be involved. I comment all my code for this reason, plus it is always nice to have something to refer to if I have to jump back into my old code. - James Campbell
(8) If you stick to using comments for explaining "why" and not "what", then I believe that you generally avoid all those annoying comments that don't add anything to the code. - Jan Aagaard
(9) From Steve McConnell's Code Complete 2 "The three kinds of comments that are acceptable for completed code are information that can't be expressed in code, intent comments and summary comments." Being dogmatic about comments is detrimental to the problem-solving aspect of development. A hint, or in some cases an explanation of the problem is going to help the reader of the code. Granted, there's lots of bad comments out there, but that doesn't make them intrinsicly bad. - StuperUser
"Only a sith deals in absolutes." - StuperUser
As an instructor I had several years ago would answer pretty much any question in class: "It depends." I've worked with maintenance on extremely huge products, and someone making a summary of what was changed in response to a bug, and probably commenting out the old code so the bug doesn't get reawoken in a few years--excellent use of comments. OTOH, look at CodeProject.com and you'll see several samples where the writer seemed to have Comment Tourette's Syndrome, only every 8th line is actual code. BAD BAD BAD! - Jay
Let me add "Properly Commented code" - James Campbell
93
[+3] [2010-10-09 20:46:41] y34h

Doing

HyperLink_Back.NavigateUrl = Request.UrlReferrer.ToString();

without checking

Request.UrlReferrer != null

94
[+3] [2010-07-05 10:56:39] Emrah GOZCU

I see codes that use unnecessary methods calls. I've done that before, but with a little care, i think we can make it better.

here is a little example;

    public string TestToString(int param1, int param2, object param3) {
        string result = String.Format("{0}-{1}-{3}", param1.ToString(), param2, param3.ToString());
        return result;
    }

Calling ToString() is really not necessary for param3, it is clear you can see with a help of ildasm.

.method public hidebysig instance string 
        TestToString(int32 param1,
                      int32 param2,
                      object param3) cil managed
{
  // Code size       37 (0x25)
  .maxstack  4
  .locals init (string V_0,
           string V_1)
  IL_0000:  nop
  IL_0001:  ldstr      "{0}-{1}-{3}"
  IL_0006:  ldarga.s   param1
  IL_0008:  call       instance string [mscorlib]System.Int32::ToString()
  IL_000d:  ldarg.2
  IL_000e:  box        [mscorlib]System.Int32
  IL_0013:  ldarg.3
  IL_0014:  callvirt   instance string [mscorlib]System.Object::ToString()
  IL_0019:  call       string [mscorlib]System.String::Format(string,
                                                              object,
                                                              object,
                                                              object)
  IL_001e:  stloc.0
  IL_001f:  ldloc.0
  IL_0020:  stloc.1
  IL_0021:  br.s       IL_0023
  IL_0023:  ldloc.1
  IL_0024:  ret
} // end of method Test::TestToString

And for param1 is not that neccessary at all, let suppose it is a type of decimal, still you don't have to call ToString if you don't need to display culture specific representation.

In any case boxing will work faster than calling ToString() method. It matters if you call that kind of methods soo often in short period.


Any other specific examples that you can think of? - James
(1) @James: You didn't like this example then :) I need to think about it. - Emrah GOZCU
(1) Ok! Here is another one: Suppose you have a listbox and you set the datasource of the control on each postback, instead it is easy to check if it is not postback than set the datasource only once, because you already set the wievstate to on for that control or for the page which your control in. This way, you don't go any datasource and query it each time you postback. It is still faster :) I think this one is more clear, right? - Emrah GOZCU
95
[+3] [2011-01-02 23:27:47] k3b
  • Stop testdriven development (TDD) because of big deadlines to "save time". If you are in a hurry to meet the deadline the chances are quite high, that you make more errors than usual. Unittest can prevent you from these errrors. I like @benzado-s Unit testing is a lot like going to the gym [1] as a good example for this.
  • not reqularly executing the unittests.
  • lost unittests because of not maintaining/updating unittests when libraries changed
[1] http://stackoverflow.com/questions/67299/is-unit-testing-worth-the-effort/69263#69263

96
[+3] [2008-12-21 03:10:24] community_owned

Also don't forget all the common code smells:

http://en.wikipedia.org/wiki/Code_smell


(1) God I hate that name, "Code smell". - TraumaPony
97
[+3] [2009-08-08 01:17:50] Addys

Boxing/unboxing is one of the common oversights of new developers. This was a more serious problem in the early days of .NET (before generics), but it is still quite easy to create innocent-looking code which is massively unefficient due to implicit boxing/unboxing.


98
[+3] [2009-07-27 19:30:57] BBetances

Not catching any errors at all, or handling them wrong. I know some developers who will write whole programs and not catch any errors, then wonder why the s*** crashes. Maybe the bigger mistake is not knowing your development language thoroughly enough.


my s*** doesn't crash because good programmers don't make exceptions and it needs a genius to understand. - Behrooz
At least On Error Resume Next is dead. - Bobby
On the other hand it's just bad to catch the errors and continue on if you don't fully understand the consequences of continuing in the state your program is in. - dwidel
99
[+3] [2009-05-17 05:40:23] andrey.tsykunov

Here [1] is a good set of guidelines and best practices from Juval Lowy (founder of IDesign).

Most of them are presented as facts without justifications, but answers can be found in his book " Programming .NET components [2]".

Although some statements can be considered as too strict and arguable I think it's worthwhile for everyone to run through his list and think about them.

[1] http://www.idesign.net/idesign/download/IDesign%20CSharp%20Coding%20Standard.zip
[2] http://rads.stackoverflow.com/amzn/click/0596003471

100
[+3] [2009-05-02 02:52:03] GregC

I saw this one before: a programmer is assigned a 'feature' that results in an exception being thrown inside a catch. He proceeds by implementing another catch, that does nothing:

Original code:

try
{
    // Do something
    // Cause an exception
}
catch(Exception e)
{
   Logger.Log(e.Message); // throws
}

Fixed code:

try
{
    // Do something
    // Cause an exception
}
catch(Exception e)
{
   try
   {
   Logger.Log(e.Message); // throws
   }
   catch { }
}

We never saw the bug again, until customer looked at the final product. Need I say more.


(5) From what I've seen, it's pretty common practice to ignore errors that occur during error logging. Why is this bad? How should the situation be handled instead? - Sander
1. If logger is yours, have a fail-safe method to log exceptions in the logger. Also, make sure that logger catches its own exceptions. Common practice is to log to different files with file names that include thread name and process name. This will help to avoid threading issues. 2. If logger is someone else's... i don't know. Make your own, I guess. - GregC
(1) So your logger code completely sucks and can't be trused. The problem should never have occured in the first place. Poor example - Mahol25
If logger can't write to its normal output, maybe it should have a secondary output, just for failures during logging. A simple message box won't do, especially if you're running on a headless server. - GregC
A mistake to avoid -- catch-all blocks that do nothing. - GregC
101
[+3] [2008-12-22 20:00:56] etsuba
  • Using string literals repeatedly instead of string constants.

  • Accessing a single list item repeatedly instead of getting it once and assigning it to a local variable.

  • Using less efficient string comparisons like string1==string2 instead of more efficient ones like string.Equals, StringComparer.

  • Always making classes and methods public even for those that should be internal to a library or a class.


(3) String.Equals is not much faster unless you are doing it loads, and it doesnt read as well. Premature optimization! - Jim
102
[+2] [2011-04-02 12:40:36] ramezani.saleh

I think this one of common mistakes.

foreach(Form frm in Application.OpenForms)
{
   frm.Close();
}

this code will raise an exception and you must use

for(int Index=0;Index< Application.OpenForms.Count;Index++)
    {
       Application.OpenForms[Index].Close();
    }

.


103
[+1] [2011-05-04 21:52:15] Jakub Linhart

Some random mistakes:

1) Session is HttpSessionState in this example:

int something = int.Parse(Session["something"].ToString());

better:

int something = (int)Session["something"];

2) Invoking event in multithreaded application:

if (MyEvent != null) {
   MyEvent(this, EventArgs.Empty);
}

better:

var handler = MyEvent;
if (handler != null) {
   handler(this, EventArgs.Empty);
}

Check Eric Lippert's blog [1] for details.

3) Unsafe locking:

lock("string") { }
lock(typeof(SomeType)) { }

See MSDN [2].

[1] http://blogs.msdn.com/b/ericlippert/archive/2009/04/29/events-and-races.aspx
[2] http://msdn.microsoft.com/en-us/library/c5kehkcz.aspx

104
[+1] [2011-05-31 11:33:58] makerofthings7

Although this will work fine in run-time, the debugging experience will be horrible if you don't use try...catch statements in your Async methods

Excerpt from this SO question [1]

Analysis:

For your information, CLR will re-throw the exception inside the callback when you call EndInvoke(). Below is a simplified version of EndInvoke():

public object EndInvoke(IAsyncResult asyncResult)
{
    using (new MultithreadSafeCallScope())
    {
        ThreadMethodEntry entry = asyncResult as ThreadMethodEntry;
         ............
        if (entry.exception != null)
        {
            throw entry.exception;
        }
     }
}

The exception will be handled in the call back function or in the asynchronous method if an exception handler is provided. This is how it works without a debugger attached.

When you run it in VS.NET, the debugger seems checking only the presence of the exception handler in the asynchronous method. If there is no such handler, the debugger would think the exception is not handled and pop up an error message notifying you of this.

Suggestion:

The application should work as expected when you run it stand alone. If the error message is annoying in debugging for you, you can disable it by unchecking “User unhandled” for “Common Language Runtime Exceptions”in the Exception dialog box (Debug|Exceptions or press CTRL+ATL+E). Or you can add try/catch in the asynchronous method. In the latter case, the exception is set to null and won’t be re-thrown in EndInvoke().

[1] http://stackoverflow.com/questions/5623456/getting-an-unhandled-exception-in-vs2010-debugger-even-though-the-exception-is-ha/6186692#6186692

105
[+1] [2010-05-21 17:39:54] dretzlaff17

Be careful when calling calling exception handling methods in your catch statement, where the exception handling code throws and exception itself. You will be caught in an continuous loop. I found this issue a number of times through the years.


106
[+1] [2009-07-22 12:57:39] RameshVel

One of the worst things even the experienced programmers are also doing is assigning database null values to variables without verifying it.

-- Missing database null verifications

Always perform a check on database operations,

(drOutput[iMainIdentifier] != Convert.DBNull) ? drOutput[iMainIdentifier].ToString() : null;   

--Accessing the fields in nulled object without checking

--Calling the nulled object without initializing it

--In UI applications, using a non-UI thread to update the UI

--Using unavailable column names in the datareader getordinal() methods

--Missing breaks in case staments..

--Break your loops carefully. There are scenarios we are looking for a specific item within a loop for further processing and letting the loop continue even if the specified item is already identified..


107
[0] [2011-03-04 11:16:58] scorpio

Using thread from thread pool and forgetting they might have data persisted from its last use

Is thread-local storage persisted between backgroundworker invocations? [1]

[1] http://stackoverflow.com/questions/561518/is-thread-local-storage-persisted-between-backgroundworker-invocations/3413896#3413896

108
[0] [2011-08-10 17:52:22] townsean

A mistake I made several times without realizing it: Not checking a Nullable [1] type's HasValue property before accessing Value.

This was a bad habit of mines prior to using WPF frequently. I got stung when I started using properties like IsChecked [2] on the WPF CheckBox. IsChecked is of type bool? to represent three states: true, false, and null.

Bad approach:

if(checkBox.IsChecked.Value)
    // do something

Good approach:

if(checkBox.IsChecked.HasValue)
{
    // do something with checkBox.IsChecked.Value
}
else
{
    // value undefined or indeterminate
}
[1] http://msdn.microsoft.com/en-us/library/2cf62fcy%28v=vs.80%29.aspx
[2] http://msdn.microsoft.com/en-us/library/system.windows.controls.primitives.togglebutton.ischecked.aspx

109
[0] [2011-09-01 06:07:38] dodgy_coder

Assuming DateTime.Now has a resolution of 1 millisecond or better. In fact the DateTime.Now has a resolution of around 15 milliseconds due to the windows scheduler. A solution to this is provided by the detailed answer to this question [1], which uses a StopWatch object to add ticks to the starting DateTime, with a 10 second idle timeout.

[1] http://stackoverflow.com/questions/1416139/how-to-get-timestamp-of-tick-precision-in-net-c

110
[0] [2011-04-27 12:21:04] StewieFG
 // When not using string builder then always use this way like to make sure we are initializing with empty string

string name= string.Empty;


111
[-3] [2010-07-29 07:50:35] xport
class Foo : IDisposable
{
    public int Denominator { private get; set; }
    public int DoDivision(int x)
    {
        return x / Denominator;
    }
    public void Dispose()
    {
        Console.WriteLine("I do nothing in Dispose method.");
    }
}

The following is not wrong but...

class Program
{
    static void Main(string[] args)
    {
        try
        {
            using (Foo f = new Foo())
            {
                f.Denominator = 0;
                f.DoDivision(1);
            }
        }
        catch (Exception ex)
        {
            Console.WriteLine(ex.Message);
        }
    }
}

Why not do like so:

class Program
{
    static void Main(string[] args)
    {
        Foo f = null;
        try
        {
            f = new Foo();
            f.Denominator = 0;
            f.DoDivision(1);
        }
        catch (Exception ex)
        {
            Console.WriteLine(ex.Message);
        }
        finally
        {
            if (f != null)
                f.Dispose();
        }
    }
}

(1) And why does it wrong ? Personally I also prefer the second one, but what is objectively wrong in the first version ? I don't see any arguments. - Incognito
(1) the first version is not wrong. - xport
(9) the using version is much cleaner in my opinion then the try finally version - Earlz
@Earlz, but with using you need try-catch enclosing it. Why not use try-catch-finally at the same time? - xport
(3) When 'new' and 'dispose' need to be paired together, 'using' does the job in one line and makes it explicit that the 'new' needs 'disposing'. Using a 'finally' to include a dispose just because you happen to have a 'try/catch' in the code already isn't really have much of an argument for doing it. The version with the 'finally' has extra complexity because it includes some extra conditional 'if' logic. - Scott Langham
112
[-4] [2010-06-24 02:11:39] Martin Ongtangco

Another bad practice is with poor use of StringBuilders. Some people append a break line by applying a new line code inside the string when they can actually just use .AppendLine.

example: BAD

StringBuilder sb = new StringBuilder();
sb.Append("first line \n");
sb.Append("second line \n");

or

StringBuilder sb = new StringBuilder();
sb.Append("first line <br/>");
sb.Append("second line <br/>");

example: GOOD

StringBuilder sb = new StringBuilder();
sb.AppendLine("first line");
sb.AppendLine("second line");

or

StringBuilder sb = new StringBuilder();
sb.AppendLine("first line");
sb.AppendLine("second line");
Response.Write(sb.ToString().Replace(Environment.NewLine, "<br/>"));

Unfortunately if use use AppendFormat you are forced to either use the bad method, have a blank AppendLine following it or create an extension method. - John
(2) Purpose of the StringBuilder is to avoid useless memory copying, while a string is concatenated. All 4 examples confirm to this. Nothing breaks by inserting the newlines differently. It's like discussing you should be writing if (! ..) instead of if (..). - vdboor
@Mark: write an extension method for those :-) - Jeroen Wiert Pluimers
(1) @vbdoor: \n is different from Environment.NewLine on certain platforms, so better go for the platform-neutral Environment.NewLine - Jeroen Wiert Pluimers
@Jeroen Pluimers: good point, so sb.AppendLine() it is. - vdboor
(6) Calling Replace is a horrible idea. - SLaks
@Jeroen Pluimers: I think you got the idea, so why do they down-vote still? We should be able to down-vote some of that down-voters who have no idea about what they down-vote! And than i saw the last line of the answer which shows this thing went too crazy... My bad down-voters :) - Emrah GOZCU
@jeroen, everyone wants to be the expert. - Martin Ongtangco
(1) @Martin Ongtangco: Few of them may succeed. Hope you get there :) - Emrah GOZCU
@emrah, thanks! - Martin Ongtangco
(1) -1 you should never be writing HTML in C#! - DaveDev
113
[-4] [2008-12-19 18:48:31] Chris

Avoid thinking you're a Hero and ask those around you for help!

Or doing this:

if ( SomeValue == null )
SomeObject.Value = null;
else
SomeObject.Value = SomeValue;

Instead of this:

SomeObject.Value = SomeValue ?? null;

EDIT: My point is, don't write three lines when one will do.

EDIT, from GordonG:

I think what he meant to say was...

Or doing this:

if ( SomeValue == null )
    SomeObject.Value = OopsSomeValueWasNull;
else
    SomeObject.Value = SomeValue;

Instead of this:

SomeObject.Value = SomeValue ?? OopsSomeValueWasNull;

(11) Why use the null coalescing operator if you're going to put null on the right-hand side? You could just do "SomeObject.Value = SomeValue". - P Daddy
Yeah, no difference at all. - Ed S.
SomeObject.Value = PossiblyNullValue ?? SomeValue would make more sense. - Daniel Earwicker
Corrected your post. I think that is what you meant. - Alex Baranosky
114
[-6] [2010-06-01 14:45:47] stormianrootsolver

Here my favorite pet peeves:

a) Writing C++ style or (even worse) Java style programs

b) Premature optimization

c) Classes with more than one responsibility

d) Ignorance towards design patterns

e) Ignorance towards language and CLR features

f) Use of P/Invoke because of ignorance instead of necessity

g) "Yoda me desire like": if (5 == x)

h) "Walking like an egyptian" ({ on the same line as the introducing statement, like those Java people do it for some reason)

i) No curly braces - code without that is UNREADABLE

j) Procedural code, especially methods with more than 3 lines of code

k) Ignorance towards modern, functional language features

l) Copy & Paste

m) Code in places where it doesn't belong


(14) Methods with more than three lines of code? How exactly is one supposed to avoid these? - Will Vousden
It's very easy to do, Will. Just think in an object oriented way and forget about procedural coding. Most of my methods are shorter than that. Same goes for my classes. I consider a class with more than a single concern a terrible code smell. - stormianrootsolver
(2) I'm still sceptical. Object oriented programming and imperative programming (I presume this is what you mean?) are complementary disciplines, not alternatives. How would you go about condensing this? pastebin.com/n67gPNrA - Will Vousden
(6) @Will @storm just because your methods are 3 lines doesn't mean you have a good design. If you need methods named write_foo, write_foo_part1 and write_foo_part2 to keep in line with your "convention" then you're doing it wrong. - Earlz
(2) @Earlz @storm: Precisely. Sometimes you have to do something which is inherently complicated and can't (or shouldn't) be broken down into logically independent steps. Promoting logic that would naturally be expressed imperatively to objects/classes is just an abuse of OOP. Things like while, if, switch, foreach, etc. exist for a reason. To arbitrarily limit yourself to three lines per method is senseless. - Will Vousden
(1) point j: you can't be serious. - Igby Largeman
(7) Three lines? You joking right, A try/catch statement is more than that. - Jamie Keeling
I still stand by it - if you need more than three lines of IMPERATIVE code (try / catch doesn't count), then something is wrong. And there is NO need for write_foo, write_foo_part1, etc... "switch", btw, is a bad code smell in the object oriented age. Smells like VB 6.0. - stormianrootsolver
@Will Thank you for your example. The micro - architecture I would apply here: a) In the beginning you have two "if" - statements. They form part of the contract, where it's even possible that "IsEmpty" is invariant. Therefore they don't count, but they should be rewritten using CodeContracts. b) Coding the whole loop strictly functional (LINQ) in one line IS possible if you add new extension methods - this would lead to a better, more sophisticated library for yourself Done. - stormianrootsolver
In a shorter form: If one needs methods with more than three lines of code, it's not a problem what he is doing in the method but more a problem of what he is doing in general. Thats my point. Needing endless chunks of imperative code is a sign that something is terribly wrong with ones knowledge about software development. - stormianrootsolver
Those damn Java people, don't know sh*t about braces. :E - Arnis L.
Btw, I do agree about those 3 lines. I myself do not follow this rule zealously though. Too lazy. And that's not an irony. - Arnis L.
About g), Yoda-ing is useful for preventing a class of bugs in C-inspired languages where you accidentally use the assignment operator = instead of the equality operator ==. You can't assign to a literal, so it'll throw a warning if you ever make this mistake. - Rei Miyasaka
115
[-17] [2008-12-20 01:58:55] community_owned

using leading (or trailing) underscores to denote member data (or anything else)

class HowToMakePeopleHateYou
{
  string _doShit;
  double _likeThis;
}

There are a total of probably 10 people in the world who prefer to read code formatted that way. Chances are, your co-workers will not be among them.

Same goes for Hungarian notation, or any other punctuation meant to imply type or role of a variable.


and there are a million people in the world who use m_ for member variables and similar. You need to stop annoying them with your preference :) - gbjbaanb
(1) These days, notation that denotes type or role is frowned upon because code gets refactored quite often. The last thing you want to do is have to change your intSpeed to be called floatSpeed. Prefixes typically just clutter up a variable name, making it just that bit more difficult to read. - Soviut
i don't agree; using hungarian notation makes reading through other people's code much easier. If the complaint is that code is refactored often why not just use the rename functionality built in.... - Gordon Carpenter-Thompson
I consider hungarian notation a nuisance; i don't need to know the type of an object because IntelliSense tells me. - Dmitri Nesteruk
(21) Actually, this is the recommended way of doing things in .NET from most of what I've read. Plus I like the fact it denotes a private field instead of a regular old variable. And it's infinitely better than m_ or Hungarian notation. - WayneM
(19) -1 I prefer the underscore for instance variables. Easier to read. - TheSoftwareJedi
(7) I'm with @TheSoftwareJedi on this, the underscore makes it easier to read and maintain which in turn makes my life easier. the only time I have had a use for Hungarian Notation is with asp.net controls because it makes it easier to find them with IntelliSense - Bob The Janitor
Agree 100% with @Bob-The-Janitor - Christian Hagelid
I personally hate the whole underscore thing. Hungarian notation I can live with for asp.net controls like Bob said. - BBetances
(3) I like m_PrivateField because of the capitalization, but I don't mind _privateField. Though naming a private field like a local variable (privateField) or a property (PrivateField) is unreadable and unacceptable as far as I'm concerned. - Allon Guralnek
(2) -1 "private string _myString" is the convention used in the .NET framework implementation itself, so I don't see why using that convention should be wrong. It's so much easier to tell local vars from private fields that way. - alexander.biskop
(1) I don't usually do "me too" posts, but in this case I want to OP to know that there are more than 10 people in the world who like using a leading underscore to denote a class variable, including me. However, I don't like Hungarian notation. - Bill W
-1 I always use _s and all my coworkers are happy with it. - Behrooz
Arghhhh... hate when people uses unnecessary and strange keywords like class. Idiots. :E - Arnis L.
Do you know that the code VB.NET generates for automatically implemented properties in VS 2010 uses this method? An auto-implemented property with name SomeProperty is stored to and retrieved from a private field _someProperty. I think you jumped too soon. - Alex Essilfie
Maybe dont use any underscores? just name your fields as you like. - Hooch
-1. Here is a partial list of people who disagree with you. People who helped write the .Net Framework Design Guidelines, work on the BCL team, etc - MarkJ
Personally I don't like the underscore thing. It reminds me of C++ times, when the underscore (or even two at the beginning) is looking naturally. I think a better way to distinguish a local variable from a class field is to use this keyword and to use it only in case when it is really hard to determine a scope of the variable. - Centro
116