Is this a legitimate alternative to the "traditional" placement pattern for a class hierarchy?
I'm not a fan of the code pattern: copy-paste reuse is potentially error prone. Even if you use code snippets or smart templates, there is no guarantee that another developer did it, which means that there is no guarantee that they did it correctly. And, if you need to see the code, you must understand it and / or save it.
What I want to know from the community is my implementation of IDispose for the class hierarchy, which is a legitimate alternative to the "traditional" placement option ? By legal, I mean correct, reasonably well executed, reliable and maintainable.
I agree that this alternative is wrong, but if it is, I would like to know why.
This implementation assumes that you have complete control over the class hierarchy; if you don't, you probably have to go back to the template. Add * () calls are usually done in the constructor.
public abstract class DisposableObject : IDisposable
{
protected DisposableObject()
{}
protected DisposableObject(Action managedDisposer)
{
AddDisposers(managedDisposer, null);
}
protected DisposableObject(Action managedDisposer, Action unmanagedDisposer)
{
AddDisposers(managedDisposer, unmanagedDisposer);
}
public bool IsDisposed
{
get { return disposeIndex == -1; }
}
public void CheckDisposed()
{
if (IsDisposed)
throw new ObjectDisposedException("This instance is disposed.");
}
protected void AddDisposers(Action managedDisposer, Action unmanagedDisposer)
{
managedDisposers.Add(managedDisposer);
unmanagedDisposers.Add(unmanagedDisposer);
disposeIndex++;
}
protected void AddManagedDisposer(Action managedDisposer)
{
AddDisposers(managedDisposer, null);
}
protected void AddUnmanagedDisposer(Action unmanagedDisposer)
{
AddDisposers(null, unmanagedDisposer);
}
public void Dispose()
{
if (disposeIndex != -1)
{
Dispose(true);
GC.SuppressFinalize(this);
}
}
~DisposableObject()
{
if (disposeIndex != -1)
Dispose(false);
}
private void Dispose(bool disposing)
{
for (; disposeIndex != -1; --disposeIndex)
{
if (disposing)
if (managedDisposers[disposeIndex] != null)
managedDisposers[disposeIndex]();
if (unmanagedDisposers[disposeIndex] != null)
unmanagedDisposers[disposeIndex]();
}
}
private readonly IList<Action> managedDisposers = new List<Action>();
private readonly IList<Action> unmanagedDisposers = new List<Action>();
private int disposeIndex = -1;
}
This is a "complete" implementation in the sense that I support finalization (knowing that most implementations don't need a finalizer), checks if an object is allocated, etc. A real implementation could remove the finalizer, for example, or subclass DisposableObject that includes the finalizer. Basically, I dropped everything I could think of only on this issue.
There are probably some edge cases and esoteric situations that I have missed, so I invite someone to poke holes in this approach or cherish it with fixes.
Other alternatives might be to use a single Queue <Disposer> delete on a DisposableObject instead of two lists; in this case, when the callers are called, they are removed from the list. There are other small variations I can think of, but they all have the same overall result: no code boilerplate.
source to share
The first problem you potentially hit is that C # only allows you to inherit from one base class, which in this case will always be DisposableObject
. Right here, you've cluttered your class hierarchy by forcing additional layers so that classes that must inherit from DisposableObject
another object can do this.
You also introduce a lot of future overhead and maintenance concerns with this implementation (not to mention the repeated training cost every time someone new enters the project and you have to explain how they should use this implementation, not specific pattern). You know there are multiple states to track with your two lists, no error handling around calls to actions, the syntax when calling an action looks like "wierd" (although it might be normal to call a method from an array, the syntax for simple input () after accessing an array looks strange ).
I understand the desire to reduce the number of templates you have to write, but a one-off remedy is generally not one of the areas that I would recommend doing downsizing or otherwise deviating from the template. The closest I usually get is to use a helper method (or extension method) that wraps the actual call Dispose()
to a given object. These calls usually look like this:
if (someObject != null)
{
someObject.Dispose();
}
This can be simplified with a helper method, but be aware that FxCop (or any other static analysis tool that checks if the implementation is correct) will complain.
As far as performance goes, keep in mind that you are making a lot of delegate calls with this type of implementation. This, by the nature of the delegate, is somewhat more expensive than a regular method call.
Maintaining uptime is definitely an issue here. As I mentioned, you have repetitive training costs every time someone new comes to the project and you have to explain how they should use that implementation and not a specific pattern. Not only that, you have problems with everyone who remembers to add their disposable objects to their lists.
Overall, I think this is a bad idea and will cause many problems in the future, especially as the size of the project and team grows.
source to share
I sometimes have to keep track of multiple open files at a time or to other resources. When I do this, I am using a utility class like the following. The object then still implements Displose () as you suggest, even keeping track of multiple lists (managed / unmanaged) is easy for developers to understand what is going on. Also, the output from the List object is not random, it allows you to remove (obj) as needed. My constructor usually looks like this:
_resources = new DisposableList<IDisposable>();
_file = _resources.BeginUsing(File.Open(...));
And here is the class:
class DisposableList<T> : List<T>, IDisposable
where T : IDisposable
{
public bool IsDisposed = false;
public T BeginUsing(T item) { base.Add(item); return item; }
public void Dispose()
{
for (int ix = this.Count - 1; ix >= 0; ix--)
{
try { this[ix].Dispose(); }
catch (Exception e) { Logger.LogError(e); }
this.RemoveAt(ix);
}
IsDisposed = true;
}
}
source to share
I like the general pattern from csharptest's answer. Having a base class available for deletion is a bit limited, but if you're using vb.net or don't mind playing with streaming static variables, a custom-designed base class will allow you to register variables for deletion even when created in field initializers or derived class constructors (usually (if an exception is thrown in the field initializer, there is no way to dispose of the already allocated IDisposable fields and if the derived class constructor throws an exception there is no way for the partially created base object to be located itself).
I wouldn't bother with your list of unmanaged resources. Classes with finalizers must not contain references to any objects not required for completion. Instead, the material needed to complete should be placed in its own class, and the "main" class should instantiate this latter class and retain a reference to it.
source to share