Using IDisposable To Unsubscribe an Event-- Do I need to Put Other Things Inside the Dispose?

Question

I have the following class

public class Presenter: IDisposable
{
   public IView View
   {get;private set;}

   //snip other object reference, all managed
   public Presenter(IView view)
  {
     View=view;
     View.MouseUp += MouseUpEvent;
  }

  public void MouseUpEvent()
  {
    //do whatever you want to do on mouse up
  }

  public void Dispose()
  {
    View.MouseUp -= MouseUpEvent;
    // no more manual disposing
  }
}

The question now is, am I implement Dispose() method correctly? Or do I need to manually dispose all the other managed objects just because I have explicilty define Dispose()?

I reckon that the GC is smart enough to do its own disposing ( except the event subscription) even without me manually doing it. Am I right?

Answer 1

Personally, I would avoid hooking/unhooking the event in the constructor and dispose. Instead I would add code to the View get/set accessors and add them there. But if the Presenter is disposed while a View is attached, I would not bother trying to clean that up. You can explicitly detach the View from the presenter if you need explicit detaching.

Having said that, here's what I know about IDisposable.

The recommended approach to implementing IDisposable is to have a protected Dispose(bool) method where you take action. The reason is, you want to distinguish between an explicit disposal and a disposal caused by finalization (garbage collection.)

When you are being disposed because of an explicit Dispose() call, it's ok to touch managed objects and you are expected to dispose of anything you've created that also needs disposing. So you do this only when disposing=true.

But if someone (you) forgets to call Dispose and the finalizer is called, you're being disposed after garbage collection (disposing=false) and you don't want to touch any managed objects because they may already be finalized. The only thing you need to free up in this case is unmanaged resources like Win32 handles and such.

Finally, when Dispose() is explicitly called you'll notice I called GC.SupressFinalize(this) which is a performance hint for the garbage collector. It lets it know that the object doesn't need to be finalized when it is collected. Finalization isn't cheap.

class MyObject : IDisposable {

    ~MyObject() {
        Dispose(false);
    }

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

    protected virtual void Dispose(bool disposing) {
        if (disposing) {
            // dispose of managed resources
        }
        // dispose of unmanaged resources
    }

}

Answer 2

If you go with the choice of subscribing in the constructor, then this looks reasonable. I would echo Josh's sentiments that it may not be the best approach. On the other hand, it may be the simplest way of achieving your goal, which is always a good thing. I'm not going to pretend to be an expert on UI patterns: having raised the concern, I'll assume that this is the way you want to work, and address the question itself :)

I personally find Josh's prescribed pattern¹ overly complex for simple scenarios - your approach is fine, with just one change: make your class sealed. If you don't want to seal the class, you should go for the Dispose(bool) option (but without the finalizer) because subclasses may also need to dispose of things, and may need a finalizer. Without the possibility of a derived type, life is simpler (as it so often is).

You don't need to do anything with other members just because you now implement IDiposable for that one reason.

So, do you need to derive any further from this class?

---

¹ I do understand that this is the standard recommended pattern, although I'd recommend that you read the advice of Joe Duffy et al for even more details - it can all get very complicated.