Dispose(ohNotThisNonsenseYetAgain)

For some odd reason, I’ve been authoring quite a few disposable types lately. Under normal circumstances, this would hardly be worth mentioning, but the recommended disposition pattern has been grating a raw nerve quite some time now. At best, it feels like a slightly nasty code smell to me. Due the volume of recent implementations, I’m starting to feel like I’m hanging out next to a sewer. I suspect that I’m starting to slip toward ignoring the guideline in favour of what I would consider a better practice, and I’m hoping that a little bit of venting might postpone that, so here it goes…

Just in case you’re not already familiar with it, here’s an outline of the implementation recommended in the .NET Design Guidelines:

public void  Dispose()
{
	this.Dispose(true);
	GC.SuppressFinalize(this);
}

protected virtual void Dispose(bool disposing)	// or private in a sealed class.
{
	if (disposing)
	{	
		// Clean up managed state here.
	}

	// Clean up unmanaged state here.
}

Seems a little odd, given that we don’t ever enter the Dispose(bool) method with a false parameter value, doesn’t it? So why bother? Well, once upon a time, we used finalizers to help clean up unmanaged state. A finalizable type ought to implement IDisposable, and the “Clean up unmanaged state here” bit in the above example should be run from both the finalizer and the Dispose() method. Ideally, we’d like to be able to write something like the following in a finalizable and disposable class:

~TheClass()	// In case you're not familiar with the syntax, this is the finalizer.
{
	// Clean up unmanaged state here.

public void  Dispose()
{
	// Clean up managed state here.

	this.Finalize();
	GC.SuppressFinalize(this);
}

Unfortunately, our code can’t invoke the finalizer directly, so we can’t call the finalizer from the Dispose() method in order to clean up the unmanaged state. To work around this, we use the Dispose(bool) method shown in the first example above, and we invoke it with a false value from the finalizer:

~TheClass()
{
	this.Dispose(false);
} 

So far, so good. However (very, very big “however”), starting in .NET 2.0, we’re not supposed to write finalizers anymore (or at least not for typical unmanaged resource clean-up). This means that we’ll never call Dispose(bool) with a false argument in the vast majority of our new disposable type implementations. In other words, there’s no practical benefit in implementing the recommended disposition pattern anymore, unless you happen to be working on one of the very rare cases where a finalizer is still required. For the vast majority of cases, where a finalizer is not required, the following would be functionally equivalent:

public virtual void Dispose()	// or non-virtual in a sealed class
{
	// Clean up managed resources here.

	GC.SuppressFinalize(this);
}

Now, let’s assume for the moment that you’ve never heard of the recommended disposition pattern. What might your naïve implementation of IDisposable.Dispose() look like? Probably an awful lot like that last example, except you would probably omit the GC.SuppressFinalize call (which wouldn’t actually matter if your type isn’t finalizable, which it shouldn’t be anyway) In other words, if the recommended disposition pattern didn’t involve a bunch of unnecessary code, naïve implementers would automatically do the right thing. This is known as the “pit of success”, and it is generally considered a good thing.

But let’s come back to you, the very well informed disposition author. You’re not naïve. You know how to implement the disposition pattern. Heck, you even have a lovely little snippet. Why should it bother you that the recommended pattern is more verbose than strictly necessary? It’s unfortunate, but every little bit of code we write is an opportunity for error. Every bit of extra complexity (including little things like a senseless redirection or one extra nested if block) increases the probability of introducing a bug and decreases the readability and maintainability of the code. It’s bad enough when things like this appear in one-off code. How much worse when we’re asked to systematically add unnecessary code as part of a recommended implementation pattern?

Would you accept the following in a code review, if the DoSomething(bool) method were never invoked with a false parameter value:

public void DoSomething()
{
	this.DoSomething(true);
}

private void DoSomething(bool allowDoing)
{
	if (allowDoing)
	{
		...
	}
}

If not, why accept a guideline that recommends the same thing? Well, obviously because it’s a guideline, and you’re a well-mannered developer who cares about exposing APIs that are consistent with what consumers have come to expect from their interactions with core .NET Framework code. But if the fact that it’s a guideline is the only reason you’re still following the recommended disposition pattern (or if you’ve already given up on following it), you might want to consider letting the folks responsible for the guideline know that you’re not too fond of it…