Disposing objects exception

Question

While crawling through sources on the web, I've encountered a lot of boiletplate code which looks the following way:

Suppose we have some public class CustomObject: IDisposable, which has a bunch of methods.

Now, each of those methods has default sanity checks:

if (inputBuffer == null)
    throw new ArgumentNullException("inputBuffer");
if (outputBuffer == null)
    throw new ArgumentNullException("outputBuffer");
if (inputCount < 0)
    throw new ArgumentException("inputCount", "< 0");

But (due to the IDisposable interface implementation) the following check is added to each method:

if (disposed)
    throw new ObjectDisposedException("MethodName");

Now - is this a common practice? Should I start reengineering my old disposable classes and implementing these checks?

Answer 1

It depends upon your usage, if in doubt it doesn't hurt to add it.

For a class intended to be used by other programs (e.g., libraries and frameworks) I would always perform this check and throw the correct exception as this will aid other applications in diagnosing errors and makes the class more robust.

For internal classes only intended to be consumed by my application you can skip the check if the error will surface quickly when calling the methods. E.g. if every method in the class used a stream, and that stream was disposed or set to null it would result in an exception pretty quickly.

If the internal class has methods that won't error though, I will always use an explicit check as I don't want a situation where some of the methods still work after the object has been disposed (except for methods that explicitly allow it such as IsDisposed).

Having the explicit check does have the advantage of explicitly documenting which methods are allowed to be called after the object has been disposed. More so if you add a comment at the top of methods that do not call GuardDisposed to state that it is allowed, then any method that doesn't start with GuardDisposed or a comment can be regarded as suspect.

To actually implement the check I prefer to move it to a separate method and use it like an assertion, e.g.

public class Foo
{
    private bool disposed;

    public void DoSomething ()
    {
        GuardDisposed ();
    }

    protected void GuardDisposed ()
    {
        if (disposed)
            throw new ObjectDisposedException (GetType ().Name);
    }
}

Answer 2

Now - is this a common practice?

Yes, it is recommended. For almost all members. If you class is IDisposable and any method that needs a resource is called after Dispose() then there is a serious logic error in the calling code. Your task is to signal this.

But note that there might be methods (or properties) that do not critically rely on the owned resource(s), they can be considered safe to call after a Dispose(). For instance an IsOpen function/property. It can simply return false, no need for an exception.

But you should not put the IsDisposed check into Dispose() iteself, the guideline is that it should be safe to call Dispose() multiple times.

Should I start reengineering my old disposable classes and implementing these checks?

In general a good idea. Whether it is worth the effort is up to you.

Answer 3

That code you put inside Dispose() (usually) method in order to be sure that it's not called esplicitly (and/or) not esplicitly more then one time on a single instance.

They use it in case when you do in that method something that can be executed one time (DeleteFile, CloseTransaction...) and any other operation you may think of, that is shouldn't executed twice in your app domain.

So if this is a common practice: I would say, it depends on your app requirements.