Am I implementing IDisposable correctly?

Question

This class uses a StreamWriter and therefore implements IDisposable.

public class Foo : IDisposable {     private StreamWriter _Writer;      public Foo (String path)     {         // here happens something along the lines of:         FileStream fileWrite = File.Open (path, FileMode.OpenOrCreate, FileAccess.Write, FileShare.ReadWrite);         _Writer = new StreamWriter (fileWrite, new ASCIIEncoding ());     }      public void Dispose ()     {         Dispose (true);         GC.SuppressFinalize (this);     }      ~Foo()     {         Dispose (false);     }      protected virtual void Dispose (bool disposing)     {         if (_Disposed) {             return;         }         if (disposing) {             _Writer.Dispose ();         }         _Writer = null;         _Disposed = true;     }     private bool _Disposed; } 

}

Is there any issue with the current implementation? I.e., do I have to release the underlying FileStream manually? Is Dispose(bool) written correctly?

Answer 1

You don't need to use this extensive version of IDisposable implementation if your class doesn't directly use unmanaged resources.

A simple

 public virtual void Dispose()  {       _Writer.Dispose();  } 

will suffice.

If your consumer fails to Dispose your object it will be GC'd normally without a call to Dispose, the object held by _Writer will also be GC'd and it will have a finaliser so it still gets to clean up its unmanaged resources properly.

Edit

Having done some research on the links provided by Matt and others I've come to the conclusion that my answer here stands. Here is why:-

The premise behind the disposable implementation "pattern" (by that I mean the protected virtual Dispose(bool), SuppressFinalize etc. marlarky) on an inheritable class is that a sub-class might hold on to an unmanaged resource.

However in the real world the vast majority of us .NET developers never go anywhere near an unmanaged resource. If you had to quantify the "might" above what probabilty figure would you come up with for you sort of .NET coding?

Lets assume I have a Person type (which for sake of argument has a disposable type in one of its fields and hence ought to be disposable itself). Now I have inheritors Customer, Employee etc. Is it really reasonable for me to clutter the Person class with this "Pattern" just in case someone inherits Person and wants to hold an unmanaged resource?

Sometimes we developers can over complicate things in an attempt to code for all possible circumstances without using some common sense regarding the relative probability of such circumstances.

If we ever wanted to use an unmanaged resource directly the sensible pattern would be wrap such a thing in its own class where the full "disposable pattern" would be reasonable. Hence in the significantly large body of "normal" code we do not to have to worry about all that mucking about. If we need IDisposable we can use the simple pattern above, inheritable or not.

Phew, glad to get that off my chest. ;)

Answer 2

You don't need a Finalize (destructor) method since you don't have any unmanaged objects. However, you should keep the call to GC.SuppressFinalize in case a class inheriting from Foo has unmanaged objects, and therefore a finalizer.

If you make the class sealed, then you know that unmanaged objects are never going to enter the equation, so it isn't necessary to add the protected virtual Dispose(bool) overload, or the GC.SuppressFinalize.

Edit:

@AnthonyWJones's objection to this is that if you know that the subclasses will not reference unmanaged objects, the whole Dispose(bool) and GC.SuppressFinalize is unnecessary. But if this is the case you should really make the classes internal rather than public, and the Dispose() method should be virtual. If you know what you are doing, then feel free not to follow Microsoft's suggested pattern, but you should know and understand the rules before you break them!