views:

267

answers:

4

I am currently working on fixing a c# codebase which does not have a good pattern of Dispose usage.

It is a large codebase, it is a resource demanding codebase and it uses many custom unmanaged c++ libraries at the low level.

I have a good understanding of the dispose pattern. I have spent some time understanding what I believe to be the gold standard article on the issue: Joe Duffy's dispose article

In an attempt to minimise code duplication, we have been considering some dispose helper classes, and so my question:

If a base class implements a standard Dispose pattern should it allow its disposed flag to be shared ie. marked as protected?

To clarify I mean should there only be a single boolean state within an inheritance heirarchy that defines whether an object instance has been disposed or should there be a private boolean at each step of the inheritance ladder?

The examples in MSDN and at the above link set a flag at each level, but never explain the reasoning behind it. I am in two minds on the issue, what are your thoughts and reasons?

A: 

I would recommend having a Disposed property on the base class with a public getter and a protected setter.

SLaks
Thanks - but to clarify why do you favour the single protected property over the private property at each level?
morechilli
Less code duplication. Since you cannot dispose each layer separately, there's no point in having separate properties. Besides, if you have separate properties, which one should be exposed to the classes' clients?
SLaks
A: 

In the case you have methods that should do something different when the class has been disposed, for example throwing an exception, I would create a protected only getter property for the base class that reads the private field on the base class. This way you allow any inheritor to know if he's able to perform an operation.

Then, for knowing if a class has already been disposed into its own dispose method (for example: to avoid releasing resources twice), I think that having a private flag is better for clarity and maintenance.

jmservera
+4  A: 

I would say no it should not share the flag. Sharing the flag creates oppuritunities for failure which better encapsulation could prevent.

For instance, consider the scenario where you have a readonly Disposed property on the base most class. The backing field is only set to true in the Dispose(disposing) method on the base class. This means that the property Disposed can only ever return true if the base class Dispose is called (barring evil reflection of course). This allows the base class to provide an enforcaeable contract.

Now consider the opposite, where there is a protected setter. Any class can now arbitrarily set the Disposed property to true without disposing anything. This creates an opportunity for Dispose to return true when nothing is disposed.

I would choose the first option because it provides the most enforceable contract.

JaredPar
A: 

In addition to JaredPar's answer, I'd add that it is not always necessary for there to be a _disposed flag. Very often, the other "resource-related" fields of the object naturally provide a way to represent the disposed state.

For example, an external COM object that needs to be Shutdown before it is discarded would be represented by a reference to the COM object, and so the relevant part of Dispose would be:

if (_comObject != null)
{
    _comObject.Shutdown();
    _comObject = null;
}

This can safely be run multiple times without calling Shutdown more than once, as is required. Other methods that attempt to use _comObject after Dispose will get a NullReferenceException, or ideally those methods would check the _comObject field and throw ObjectDisposedException.

I find this is more often true than not - having frequently implemented IDisposable, I can't recall ever needing a separate _disposed field. Introducing one would increase the degrees of freedom (increase the number of ways for me to screw it up).

So this is unlikely to be that useful for derived classes even if it was a safe idea (which it isn't, as JaredPar explains).

Daniel Earwicker