C# Corner

C#: The Good, Bad and Ugly

• By Patrick Steele
• 10/01/2010

It's almost 2011. The Microsoft .NET Framework and C# have been publicly available for almost 10 years -- longer if you count the early betas. In this article, I'll look at some of the things I love about C#, some things I could do without and those things that really should be avoided!

With apologies to Sergio Leone, I'm going to approach this in reverse order. First the ugly, then the bad -- and I'll conclude with the good.

The Ugly
Here are some really nasty things you can do in C#; they should be avoided if at all possible.

lock(this): If you've got a public class, using lock(this) is an invitation for a deadlock. Locking the current object instance seems like the easy thing to do to get thread safety, but what if other code in your class further down the stack also does lock(this)? Maybe this will only happen in a few places, but once you get your first report of a deadlock, you've got to find every lock(this) in the class and investigate call hierarchies to see where the deadlock might be occurring.

Or, what if you've got lock(this) inside your class, and one of the consumers in your class does a lock(the_instance_variable) before calling one of your methods? The consumer will have locked you out of your own code! Locking should always be done on a private object specifically used for locking.

Finalizers: I debated putting finalizers under the "bad" section instead of ugly, but I really think they should be avoided. Use the Dispose pattern instead.

What's wrong with finalizers? First off, they're non-deterministic.

You never know when the Garbage Collector (GC) will call them. In fact, calling finalizers isn't even guaranteed by the Microsoft .NET Framework. Second, finalizers put added pressure on the GC -- objects that define a finalizer are processed in a separate queue and therefore stay around longer.

If you absolutely feel you must use a finalizer to make sure some unmanaged resource is released -- which, as I just noted, you really can't ensure -- you should implement the IDisposable interface. In your Dispose method, release your unmanaged resources and call GC.SuppressFinalize(this) to make sure your object never gets to the finalization queue.

Finally (if you'll pardon the pun), remove any finalizer as soon as they're deemed unnecessary. As long as a finalizer exists in your class -- even if it's empty -- you'll still pay the GC cost of a "finalizable" object.

catch(Exception): The try/catch/finally construct is a great tool for handling issues that may arise in certain areas of code. One all-too-common abuse of the try/catch block is catching every possible exception type. Catch blocks are only meant to catch exceptions you can handle.

As an example, let's say your code is polling a document via an HTTP-based URL, someserver.net/SomeResource. You start getting errors because the site is used heavily and sometimes generates a 500. Because this isn't a super-critical error (your application can just wait for the next polling interval and try again), you wrap it in a try/catch block:

	try
	{
		// Perform an HTTP GET on some url
	}
	catch (Exception e)
	{
		// Probably a 500. Ignore it and continue
	}

By catching Exception, you're catching every possible exception the Microsoft .NET Framework could throw. Suppose the provider of the resource changes the URL and doesn't provide a redirect. Now your requests start returning a 404. With the catch block I just showed, your code would ignore it. Or maybe there's a bug somewhere else in your app that isn't disposing of TCP/IP connections properly and you're running out of connections, thus generating a different type of exception. All of these will be ignored because your code catches the general Exception class.

Always catch the most specific exception possible. In the previous scenario, if you decided that a 500 wasn't a reason to abort the call, you should look for that condition only:

	try
	{
		// Perform an HTTP GET on some url
	}
	catch (WebException e)
	{
		var response = (HttpWebResponse)e.Response;
		if( response.StatusCode == 
        HttpStatusCode.InternalServerError)
		{
			// We'll try again on the next poll
		}
		else
		{
			// Handle the issue!
		}
	}

This clearly indicates that you expect possible HTTP 500 errors and want to ignore them. Likewise, if you think the URL might change without reason, you could capture the 404 status (HttpStatusCode.NotFound) and alert the system administrator accordingly.

Catching the general Exception type says "I can handle any exception." That's utterly impossible to do, so coding for it is a really ugly thing to do.

The Bad
Let's look at a few things that can mess up your code.

Not Rethrowing Exceptions Properly: Exceptions let you know when things didn't go as planned. Exception handling in the .NET Framework is very robust, but you need to understand how the exception-throwing process works or you may not get all of its benefits. Everyone knows how to throw exceptions:

	public void Deposit(decimal amount)
	{
		if( amount < 0)
		{
			throw new ArgumentException(
           "amount to deposit can not be negative", "amount");
		}

		// ...
	}

In this example, the stack trace would indicate the exact line where the "throw" was found as the location of the error. You'd jump right there and see that someone passed the Deposit method a negative value.

But what about catching an exception and re-throwing it for the consumer to handle? If you don't do it the right way, you can lose important stack-trace information. Consider the following class which will (obviously) throw a DivideByZeroException:

	public class RethrowProperly
	{
		public void DoSomething()
		{
			var d = 0;
			var n = 15;

			var result = n/d;
		}
	}

DoSomething is dividing and doesn't always check the denominator for zero, so you wrap the call to DoSomething in a try/catch block:

	try
	{
		var r = new RethrowProperly();
		r.DoSomething();
	}
	catch (DivideByZeroException ex)
	{
		throw ex;
	}

When this code is run and the exception is thrown, the line number in the stack trace will point right to the "throw ex" line as the source of the error. But the error actually happened back in DoSomething when executing the "n/d" calculation. Because you're passing an exception to "throw," you lose the previous stack trace and start a new stack trace for the exception.

Instead, simply call "throw," and the exception is re-thrown with the stack trace intact:

	try
	{
		var r = new RethrowProperly();
		r.DoSomething();
	}
	catch (DivideByZeroException ex)
	{
		throw;
	}

It's a small but subtle difference. Now the call stack points directly at the line of code in DoSomething where the calculation is done. It's much easier to debug when you know exactly where an error is happening.

Public Fields: Public fields are just plain bad. Non-public fields allow classes and structures to store and encapsulate data, while public fields expose the data without any restrictions or checks (a public delegate can lose its entire invocation list by simply setting it to null). Publicly accessible data should always be exposed through properties.

With the separate get/set semantics of properties, you have control over how the data is stored and retrieved. And with the ability to define a different accessibility level for the "getter" and the "setter," you have even more control over how the data is exposed and manipulated.

Properties are an important aspect of the way data is exposed; WinForms data binding -- along with other utilities and libraries in the .NET Framework -- actually requires properties. Its reflection-based binding only looks for properties; it won't even look at public fields.

The Good
Now let's explore the stuff that makes C# such a pleasure to use. As I started writing this article, this list was the easiest to write.

Delegates: Explaining a delegate as simply a "type-safe function pointer" seems to do it such a disservice. Not only do delegates restrict available call sites to a specific signature, they also represent a complete invocation list that supports any number of subscribers. Another departure from function pointers is that delegates can represent instance methods as well as static methods (function pointers are limited to static methods). Finally, delegates automatically support asynchronous calling via the BeginInvoke/EndInvoke methods that are automatically generated for all delegates. Take that, function pointers!

Yield Keyword: The young whippersnappers who started using C# with the 2.0 release probably have no idea what the old folks used to have to do to create an object support for a custom enumerator. With the yield keyword, the C# compiler does all the heavy lifting of generating a little state machine behind-the-scenes. The yield keyword allows you to easily expose any set of data as an enumerable list that's processed with a simple foreach loop.

I crack a smile every time I think about all the work the compiler does on my behalf with the yield keyword.

Using Keyword: I mentioned finalizers in the "ugly" portion of this article. Dealing with unmanaged resources is something developers must deal with until we get a fully managed OS. The IDisposable pattern is the accepted way of handling and disposing of unmanaged resources. But the burden is still on the developer to call Dispose when finished with an IDisposable object.

That's where the "using" keyword comes in. Instead of writing a bunch of try/finally blocks to ensure your object's Dispose method was called, you simply wrap it in a using block. I never liked relying on try/finally blocks for resource management -- they're supposed to be used for exception handling -- so I welcomed the using block when it was introduced.

Lambdas: I left lambdas for last because they make delegates more fun to use.

Back in the .NET Framework 1.x, when you needed to use a delegate you had to create an instance of a specific delegate pointing to some method somewhere else (in the same class, in a different class -- it didn't matter). With the .NET Framework 2.0, you got anonymous delegates. These allowed you to, in a way, inline the method code. You no longer had to create a separate method -- the compiler would do that for you.

With the .NET Framework 3.x, we have lambdas. These are the abbreviated version of anonymous methods. Code that used to look like this in the .NET Framework 2.0 (converting each element of a double array to its string equivalent):

	public void ChangeData(double[] data)
	{
		string[] genericSA = Array.ConvertAll(data, 
        new Converter<double, string>(DoubleToStringConverter));

		// Do something with genericSA
	}

	private static string DoubleToStringConverter(double d)
	{
		return d.ToString();
	}

Is now reduced to a mere one-liner:

string[] genericSA = Array.ConvertAll(data, d => d.ToString());

The anonymous method -- d.ToString() -- is converted to a real method when compiled to IL. The delegate type, along with the parameter types, is inferred automatically by the compiler. You get to write more of the "what" of the code, and the compiler does more of the "how."

This is in no way an exhaustive list, but just a few things that stuck in my head when I was asked how I would describe the good, the bad and the ugly of C#. Thanks to the efforts of Microsoft and its language designers, it was rather difficult to create the ugly list.

I'm sure I've missed something you would've included in one of the lists. Do you have a good, bad or ugly C# list you'd like to share? E-mail me at [email protected].