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 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.

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)

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.

• Writing structs. You very rarely need to, and if you do, they should be immutable.
• Overriding Equals ^[1], but not GetHashCode (nor != / ==)
• Non-private fields ^[2]
• Premature optimisation (worrying too quickly about for(...) vs foreach(...) ^[3] etc)
• Not using your IDisposable ^[4] objects
• Static (mutable) data in a web app; threading nightmare...

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

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.

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.

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.

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.

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. ");

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.

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:

• http://incrediblejourneysintotheknown.blogspot.com/2009/02/fatal-exceptions-and-why-vbnet-has.html
• http://stackoverflow.com/questions/602066/why-doesnt-c-have-support-for-first-pass-exception-filtering

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.

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();

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.

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;
}

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));

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.

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;
    }
}

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.

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.

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.

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);

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

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!
}

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.

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.

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.

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.

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()
    }     
 }

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.

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

Calling GC.Collect().

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

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 ...

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.... 
}

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.

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.

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
}

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.

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.

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++)

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.

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.

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

How to resolve ^[1]

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.

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
}

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));

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

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();
    }
}

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

• 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.

• 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).

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();
    }
}

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

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;
        }
    }

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.

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!

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

Unnecessary boxing and unboxing.

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);

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).

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.
}

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.

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;

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 :)

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.

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. :)

Check out the book Framework Design Guidelines ^[1].

It contains a lot of DOs and DO NOTs.

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);

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.

 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].

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.

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.

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.

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.

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.

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.

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 ...

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.

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.

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
  {
    ...
  }
}

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);
	}

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.

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);
}

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.

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

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

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

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.

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.

        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(); 

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.

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

string foo = GetString();
if (foo == null) throw ex1;
if (string.IsNullOrEmpty(foo)) throw ex2;

foo is checked twice for null.

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

Doing

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

without checking

Request.UrlReferrer != null

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.

• 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

Also don't forget all the common code smells:

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

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.

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.

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.

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.

• 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.

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();
    }

.

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].

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().

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.

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..

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]

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
}

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.

 // When not using string builder then always use this way like to make sure we are initializing with empty string

string name= string.Empty;

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();
        }
    }
}

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/>"));

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;

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

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.