Every C# codebase has bugs that look fine to the eye but blow up at runtime, corrupt data, or hang the process. The language gives you a lot of safety, but it also has a long list of sharp edges that catch even experienced developers. This chapter walks through the pitfalls that show up most often in real code, what they look like, why they happen, and how to fix them.

The mistakes covered here share a few traits. They compile cleanly. They often pass casual testing. They look like idiomatic C# at a glance. And when they fail, the symptom is rarely close to the cause: a NullReferenceException thrown three calls deep, a deadlock on a button click, a unit test that passes locally and fails in production because the machine's locale changed.

The pattern that ties them together is assumption mismatch. The compiler assumes one thing, the runtime does another. You assume a variable can't be null, but the framework hands you null anyway. You assume == compares contents, but for the type you're using it compares references. You assume await is implicit, but you forgot it and the task just goes off and runs without anyone watching.

Once you know the shapes these mistakes take, spotting them becomes second nature. The fixes are usually small. The cost of not knowing them is a debugging session that eats a Friday afternoon.

NullReferenceException is the most common runtime exception in C#. It happens when you dereference a variable that holds null, by calling a method on it, accessing a property, or indexing into it.

What's wrong?

Behavior: Throws System.NullReferenceException: Object reference not set to an instance of an object. The method returned null, and .Length tries to read a property off nothing.

Fix: Check for null before dereferencing, or use the null-conditional and null-coalescing operators.

The ?. operator short-circuits when the left side is null, so couponCode?.Length evaluates to null instead of throwing. The ?? operator then supplies a fallback when the left side is null.

Nullable reference types (C# 8+, enabled by default in new projects) make the compiler track which references might be null. When the feature is on, a plain string means "non-null string" and string? means "might be null." The compiler then warns you if you dereference a string? without checking.

The pitfall is misusing the null-forgiving operator !. Writing couponCode!.Length tells the compiler "trust me, this isn't null." If you're wrong, the warning goes away but the runtime exception is the same.

What's wrong?

Behavior: Compiles without warnings. Throws NullReferenceException at runtime, just as if nullable reference types were turned off.

Fix: Use ! only when you have a real, provable reason the value can't be null in this context, for example, you just checked it on the previous line and the compiler can't see that. When you're unsure, check explicitly.

After the is null check, the compiler knows couponCode is non-null inside the rest of the method, so couponCode.Length is safe without !.

A related trap: a non-nullable field left uninitialized.

The compiler warns because Owner is non-nullable but no constructor sets it. If you ignore the warning, new Cart().Owner.Length throws NullReferenceException. The fix is either to require it (public required string Owner { get; set; } in C# 11+), give it a default (= ""), or take it in a constructor.

C# collections detect changes during a foreach and throw InvalidOperationException on the next iteration. The exception message is usually Collection was modified; enumeration operation may not execute.

What's wrong?

Behavior: Throws System.InvalidOperationException: Collection was modified; enumeration operation may not execute. The List<T> enumerator tracks a version number that bumps on every mutation, and detects the mismatch on the next MoveNext.

Fix (option 1): Iterate over a snapshot, mutate the original.

cart.ToList() builds a new list with the same items. Iterating that snapshot leaves the original free to mutate.

ToList() allocates and copies the whole collection. For small carts that's fine. For collections with thousands of items, prefer the in-place version below.

Fix (option 2): Use a backwards index loop, or RemoveAll.

RemoveAll walks the list once, shifts elements in place, and avoids the snapshot copy. Use it when the predicate captures the "what to remove" logic cleanly.

The same trap exists for Dictionary<TKey, TValue>, HashSet<T>, and Queue<T>. All of them throw on concurrent modification during enumeration. The fix is the same: iterate a copy of the keys, or use a method designed to filter in place.

A subtler version: reading from a dictionary while another thread writes to it. Dictionary<TKey, TValue> is not thread-safe; you need ConcurrentDictionary<TKey, TValue> (in System.Collections.Concurrent) for that case.

The return type of an async method is normally Task or Task<T>. async void is a special form reserved for one purpose: matching the signature of an event handler. Anywhere else, it's a bug waiting to happen.

What's wrong?

Behavior: The call returns immediately because async void can't be awaited. The program prints After processing and exits. When the delayed exception fires, it's raised on a thread-pool thread with no one to catch it, and in many hosts it crashes the process.

Fix: Return Task so the caller can await it, and so exceptions surface to the awaiter.

The only legitimate async void is an event handler:

WinForms, WPF, and ASP.NET event subscriptions need a void-returning delegate, so async void is the only way to write an async event handler. Even there, you should wrap the body in try/catch, because exceptions out of an async void method are raised on the synchronization context and typically crash the app.

The rule of thumb: write async Task for methods that don't return a value, async Task<T> for methods that do, and async void only when the framework forces you to.

Two related mistakes around async. The first is forgetting await.

What's wrong?

Behavior: Prints Saved and may exit before SaveOrderAsync finishes. The unawaited Task is dropped on the floor. Any exception inside it becomes an unobserved task exception, silently logged or, depending on framework settings, swallowed entirely.

Modern compilers give you a warning (CS4014: Because this call is not awaited, execution of the current method continues before the call is completed) but the code still compiles. Treat that warning as an error.

Fix: Add await, or be deliberate about fire-and-forget by handling exceptions inside and assigning to _.

The second mistake is calling .Result or .Wait() to "make" an async method synchronous. In a UI or classic ASP.NET context with a captured synchronization context, this deadlocks.

What's wrong?

Behavior: In a UI app or classic ASP.NET context, this deadlocks. The caller blocks the synchronization context waiting for the task. The task's continuation, the code after await, needs to run on that same context to return the result. Neither can move forward.

The shape of the deadlock:

The UI thread waits for the continuation, the continuation waits for the UI thread. Classic circular dependency.

Fix: Make the caller async and await the task, all the way up. Async is contagious by design.

When you absolutely cannot make the caller async (an interface forces a sync signature, for example), the safest workaround is to push the work to a separate thread with Task.Run(() => GetCustomerNameAsync(id)).GetAwaiter().GetResult(). But that's a code smell, not a pattern to spread. ASP.NET Core does not have a synchronization context, so the deadlock doesn't happen there, but blocking threads on async work still hurts throughput.

Types that implement IDisposable hold unmanaged resources (file handles, sockets, database connections, native memory). Failing to dispose them leaves the resource open until the finalizer runs, which might be minutes later, or never if the process exits abruptly.

What's wrong?

Behavior: The file handle stays open until the garbage collector finalizes the object. If the loop opens hundreds of files, you can hit the OS limit on open handles and get IOException: Too many open files or similar. On Windows, the file is locked and other processes can't read it.

Fix: Use a using declaration or using block.

The using declaration (C# 8+) disposes the resource at the end of the enclosing scope. The older form wraps the resource in a block:

Both compile to the same try/finally: the disposable's Dispose() is called even if the body throws.

A second pitfall is double dispose. Dispose is supposed to be idempotent, but not every type implements it that way. Returning a disposable from a method and then wrapping the same instance in another using causes problems.

What's wrong?

Behavior: StreamReader disposes its underlying stream by default. The outer using var stream then disposes the already-disposed stream. For FileStream, this happens to be a no-op, but for other types it can throw ObjectDisposedException. Worse, if you keep a reference to stream after the inner using scope, every operation on it throws.

Fix: Hand ownership to whichever wrapper will live longer, and use the constructor overload that says "don't dispose the inner stream" when you need both.

The leaveOpen: true parameter tells StreamReader not to dispose the stream when it itself is disposed. The outer using then disposes the stream exactly once.

The third pitfall is the finalizer. A finalizer (~MyClass()) is the GC's last-resort cleanup, used for unmanaged resources. Most code should not write one. If you do, the rules are strict: the finalizer must call a private Dispose(bool disposing) overload, never call methods on managed objects (they may already be finalized), and call GC.SuppressFinalize(this) from Dispose() so the finalizer doesn't run twice.

A safer default: avoid finalizers entirely. If your type owns an unmanaged resource, wrap it in SafeHandle (from Microsoft.Win32.SafeHandles), which gets the finalizer right for you.

Closures capture variables by reference, not by value. When the captured variable changes after the closure is created, the closure sees the new value.

What's wrong?

Behavior: All three actions print Item index 3. Each closure captures the same i variable. By the time the actions run, the loop has finished and i is 3.

C# 5 changed the behavior of foreach so that each iteration gets a fresh variable. The for loop above still captures the same i, though, because i is declared once outside the loop body.

Fix: Copy the loop variable into a local inside the loop body.

Each iteration creates a fresh captured local. Each lambda captures its own copy, and the values are preserved.

This used to bite hard with foreach too, but as noted, C# 5 fixed foreach specifically. Any for, while, or do-while loop still has the original problem, and a for loop is the more common case in practice.

The trap shows up in real code when registering event handlers, queuing tasks, or building LINQ chains with deferred execution.

Boxing happens when a value type (int, decimal, struct) is converted to a reference type (object, an interface, or dynamic). The runtime allocates a new object on the heap, copies the value into it, and hands you a reference. Unboxing is the reverse: copy the value back from the heap into a value-type variable.

The bug here isn't the allocation cost. The correctness bug is that boxing produces a copy, which means changes to the original don't show in the boxed version.

What's wrong?

Behavior:

The boxed object is a separate heap copy of the original 10. Mutating stock doesn't touch the boxed value.

The shape of boxing in memory:

The same trap fires whenever a value type ends up in a non-generic collection (ArrayList, Hashtable), in an object parameter, or in dynamic.

Each Add call boxes. Each cast back to int unboxes. If you mutate first afterward, the value in the list doesn't change because they're separate copies.

Fix: Use the generic collection List<int> instead, which stores int directly without boxing.

List<int> keeps int values in a typed array. No boxing happens. The semantics are still "copy on read" for value types, so mutating first won't change ages[0], but at least the types match and the cast is gone.

Implementing an interface on a struct is another sneaky boxing point. Calling an interface method on a value-type variable through an interface reference boxes the struct, because the interface call needs an object reference.

If DoWork mutates the struct, the change happens on the boxed copy, not on myStruct. Mutable struct gotchas are covered below.

string.Equals, string.Compare, ==, StartsWith, and friends each have a default comparison rule. Some default to ordinal (code-unit by code-unit), some to culture-sensitive (locale-aware). Mixing them produces bugs that only show on machines with certain locales.

What's wrong?

Behavior:

product is "Café" written with the single Unicode character é (U+00E9). search is "Cafe" followed by U+0301 (combining acute accent). They render identically. Ordinal comparison treats them as different code-unit sequences. Culture-sensitive comparison normalizes the characters and treats them as equal.

A more famous case: the Turkish locale.

On tr-TR, ToLower() converts uppercase I (U+0049) to dotless lowercase ı (U+0131), not to i. So "FILE.TXT".ToLower() becomes "fıle.txt", which is not equal to "file.txt". Code that worked on a US English laptop fails when deployed to a Turkish machine.

Fix: Be explicit. Use StringComparison.Ordinal for identifiers, file names, dictionary keys, anything where you want code-unit equality. Use StringComparison.OrdinalIgnoreCase for case-insensitive identifier matching. Use StringComparison.CurrentCulture only for user-facing display sorting.

The rule of thumb:

== and string.Equals(other) use ordinal comparison. string.Compare(a, b) defaults to culture-sensitive. That mismatch alone has caused many production bugs. Pass the StringComparison enum explicitly and the ambiguity goes away.

In C#, types control their own notion of equality. == on a reference type compares references by default (are these the same object?), while Equals may be overridden to compare contents. Forgetting which form a type uses is a common bug, especially for custom classes.

What's wrong?

Behavior:

Both == and Equals for a class default to reference equality: two different instances are never equal, even if every field matches. If you intended a and b to be considered the same product, you have to override Equals and GetHashCode.

Fix: Override Equals and GetHashCode together, or use a record which generates them for you.

record types (C# 9+) generate value-based Equals, GetHashCode, and ==/!= for you. Two records are equal when every property matches.

For non-record classes, the manual override looks like this:

The contract that often gets broken: if `a.Equals(b)` is true, then `a.GetHashCode() == b.GetHashCode()` must also be true. Overriding one without the other breaks Dictionary<TKey, TValue> and HashSet<T>, because they use the hash code to find a bucket and only then call Equals to confirm.

What's wrong?

Behavior: Prints contains: False. The two Product instances have different reference-based hash codes, so HashSet<T> puts them in different buckets and never even calls Equals. The override is silently useless.

Fix: Override both, always together. Most modern IDEs warn you with a compiler warning (CS0659) when you do one without the other.

A related rule: a type's hash code must not change while it's used as a key in a hash-based collection. If Sku is mutable and you change it after adding the product to a HashSet, the hash bucket no longer matches the current hash, and the entry becomes unreachable. Make the fields involved in equality immutable (use init-only setters or readonly fields).

Structs are value types. Every assignment, parameter pass, and property read produces a copy. When the struct has mutable state, that copy semantics surprises people.

What's wrong?

Behavior: This doesn't even compile. The expression cart.ItemCount returns a temporary copy of the struct from the property getter. Calling Increment() on that temporary would mutate something you can't see, so the compiler refuses with CS1612: Cannot modify the return value of 'Cart.ItemCount' because it is not a variable.

That error is the compiler doing you a favor. If ItemCount were a public field instead of a property, the code would compile and silently fail.

What's wrong (compiles, behaves wrong)?

Behavior:

When the field is readonly, the compiler creates a defensive copy on every access to protect the original from mutation. Each Increment call modifies a throwaway copy. The field stays at zero forever.

The visual:

The struct's mutation operates on a copy that vanishes the moment the call ends.

Fix: The advice from the C# team and the .NET design guidelines is unambiguous: don't write mutable structs. Make struct fields readonly, use readonly struct (C# 7.2+) to enforce it at the type level, and expose new values through methods that return a new struct instead of mutating.

The struct is immutable, the mutation pattern is replaced with reassignment, and the readonly-field trap can't happen.

Common BCL types follow this pattern: DateTime.AddDays returns a new DateTime instead of mutating, TimeSpan is immutable, decimal is immutable. The exceptions (List<T>.Enumerator, for example) are documented as "do not copy."

Catching Exception (the base type of every exception) is almost never right. It collapses every error into "something went wrong" and prevents the runtime from telling you what.

What's wrong?

Behavior: If the file is missing, an OutOfMemoryException happens, the disk fails, or some other catastrophic error is raised, this code silently continues as if nothing happened. The result is corrupted state, missing data, and a process that limps along long enough to do real damage before crashing somewhere far away from the cause.

Fix: Catch only the exceptions you can actually handle, and rethrow (or log and rethrow) everything else.

Output (when the file is missing):

When you do need to catch broadly (a top-level error boundary in a web request handler, say), at minimum log the exception before deciding what to do with it.

A related anti-pattern is throw ex; instead of throw;.

What's wrong?

Behavior: Compiles fine. Looks identical to throw; on a quick read. But throw ex; resets the stack trace to the line of the rethrow. The original stack, which is what tells you where the problem started, is lost.

Fix: Use throw; (no expression). It preserves the original stack trace.

The when filter (C# 6+) lets you catch conditionally without losing trace information either:

The filter runs before the stack unwinds, so the catch only fires when the condition matches. If it doesn't, the exception continues to propagate as if there was no catch.

Substring and the range syntax both take indices, and getting them wrong by one is a recurring source of ArgumentOutOfRangeException and silently wrong output.

Substring(int startIndex, int length) takes a length, not an end index. New developers reading the signature often assume the second argument is an exclusive end.

What's wrong?

Behavior:

The intent was likely to grab "BOOK" (length 4), but 5 returns "BOOK-". The off-by-one is small enough to slip past unit tests on examples like "BOOK-001" and only fail when the format changes.

Worse, requesting a length that runs past the end of the string throws:

startIndex + length must not exceed Length. Range syntax (C# 8+) makes both ends explicit and easier to reason about.

Fix: Use ranges with .. syntax, which uses start-inclusive, end-exclusive indices.

sku[..4] is equivalent to sku.Substring(0, 4). The end index 4 is exclusive: the result contains characters at positions 0, 1, 2, and 3.

The range syntax has a few useful forms:

The hat operator ^ indexes from the end, so s[^1] is the last character, s[^2..] is the last two.

A common off-by-one happens when computing a slice based on a delimiter position.

What's wrong?

Behavior:

IndexOf returns the index of the character, and Substring(0, at) already excludes it. Adding + 1 includes the @.

Fix:

Using ranges, the end-exclusive nature is right in the syntax and the off-by-one disappears.

== between float or double values rarely does what you want. Floating-point arithmetic has rounding errors that compound across operations, so two values that "should" be equal often differ by a tiny amount.

What's wrong?

Behavior:

The decimal 0.1 has no exact binary representation. Neither does 0.2 or 0.3. The closest double values, when added, produce a sum that's a hair more than the closest double representation of 0.3. The comparison fails.

Fix (option 1): Compare with a tolerance.

The tolerance (1e-9) should be chosen based on the scale of the values you're comparing. For numbers around 1, 1e-9 is fine. For numbers around 1_000_000, you'd want something larger because the absolute error grows with the magnitude.

Fix (option 2): Use decimal for money and any quantity that must be exact.

decimal stores numbers in base 10, so 0.1, 0.2, and 0.3 are all exact. The trade-off is slower arithmetic and a smaller range than double, but for prices, totals, taxes, and discounts it's the correct type to use.

The rule of thumb: use double for measurements (lengths, weights, scientific quantities, anything where small errors are tolerable), decimal for money and anything counted, float only when memory or interop forces it.

A short e-commerce snippet that hides four pitfalls covered in this chapter. Read it before scrolling to the explanation.

Spot the bugs before reading on.

Pitfall 1: Floating-point equality. 0.1 + 0.2 isn't 0.3 in double, so the if never fires for ORD-1 even though it logically "matches." Fix by storing Total as decimal.

Pitfall 2: Modifying the list while iterating. If the comparison did match, orders.Remove(order) inside the foreach would throw InvalidOperationException. Fix with RemoveAll(o => o.Total == 0.3m) or iterate orders.ToList().

Pitfall 3: async void outside an event handler. SaveOrder is fire-and-forget, exceptions inside it crash the process, and the outer loop has no idea when the saves finish. Fix by making it async Task and awaiting.

Pitfall 4: Unawaited tasks. Even after fixing SaveOrder, the foreach calls it without await, so all the saves race and the Done line prints first. Fix with await SaveOrder(order);.

The corrected version:

ORD-1 and ORD-2 both have a Total of 0.3m (because decimal arithmetic is exact), so RemoveAll strips both. Only ORD-3 survives. Each SaveOrderAsync is awaited in turn, so the loop finishes before Done prints, and any exception inside SaveOrderAsync would propagate to the caller instead of vanishing.

Four bugs, four fixes, one small file. Reading code with this checklist in mind is the most reliable way to keep these pitfalls out of your own projects.