Difference between two dispose implementations?

Question

Is there a difference between these two implementations?

1 :

public class SMSManager : ManagerBase
{
    private EventHandler<SheetButtonClickEventArgs> _buttonClickevent; 

    public SMSManager(DataBlock smsDataBlock, DataBlock telephonesDataBlock) :
        base(smsDataBlock)
    {
        _buttonClickevent = new EventHandler<SheetButtonClickEventArgs>(OnButtonClick);
        SheetEvents.ButtonClick += _buttonClickevent;

    }

    public override void Dispose()
    {
        base.Dispose();
        if (_buttonClickevent != null)
        SheetEvents.ButtonClick -= _buttonClickevent;
    }
}

2 :

public class SMSManager : ManagerBase
{
    public SMSManager(DataBlock smsDataBlock, DataBlock telephonesDataBlock) :
        base(smsDataBlock)
    {
        SheetEvents.ButtonClick += new EventHandler<SheetButtonClickEventArgs>(OnButtonClick);   
    }

    public override void Dispose()
    {
        base.Dispose();
        SheetEvents.ButtonClick -= new EventHandler<SheetButtonClickEventArgs>(OnButtonClick);
    }
}

The first one seems to be more correct than the second in regards to memory leaks. But is it really correct?

Don't forget to mark an answer as the answer, it helps enrich the content on this site. — Adam Houldsworth, May 22 '12 at 15:37

score 5 · Accepted Answer · edited May 23 '17 at 10:34

The long and short of it is the second piece of code is correct and safe (even if there are no handlers registered).

Consider this sample app:

namespace ConsoleApplication61
{
    class Program
    {
        static void Main(string[] args)
        {
            var f = new Foo();
            f.MyEvent += new EventHandler(Handler);
            f.Trigger();
            f.MyEvent -= new EventHandler(Handler);
            f.Trigger();
            Console.Read();
        }

        static void Handler(object sender, EventArgs e)
        {
            Console.WriteLine("handled");
        }
    }

    class Foo
    {
        public event EventHandler MyEvent;
        public void Trigger()
        {
            if (MyEvent != null)
                MyEvent(null, null);
        }
    }
}

This sample prints "handled" once.

So in your example, they are functionally the same and both will work as needed. Removing a handler that hasn't been added is also a safe action, it simply finds nothing to remove and does nothing.

As provided in the comments, Marc's answer goes into more detail:

Unregister events with new instance of the delegate

Event Handlers with Anonymous Methods

It is worth noting that event handlers in the form of lambda expressions are not guaranteed to enforce uniqueness based on instance and method signature. If you need to unsubscribe an anonymous method, you either need to promote it to a method or keep a reference to the anonymous method for later use:

Func<object, EventArgs> meth = (s, e) => DoSomething();

myEvent += meth;
myEvent -= meth;

Jon Skeet goes into detail answering this and likely does a better job of it than me :-)

How to remove a lambda event handler

A Slight Refactoring

I would refactor to the following:

public class SMSManager : ManagerBase
{
    public SMSManager(DataBlock smsDataBlock, DataBlock telephonesDataBlock) 
        : base(smsDataBlock)
    {
        SheetEvents.ButtonClick += OnButtonClick;   
    }

    public override void Dispose()
    {
        SheetEvents.ButtonClick -= OnButtonClick;
        base.Dispose();
    }
}

@SwDevMan81 Bingo! I've been looking for that information, ta. — Adam Houldsworth, May 22 '12 at 15:17
Thats why we all hang around here :) Always learning new things — SwDevMan81, May 22 '12 at 15:21
@All The interesting one is event handlers with anonymous methods (the new vogue), they don't play ball as nicely. I only learnt this the other day. — Adam Houldsworth, May 22 '12 at 15:22
@SwDevMan81 The one (and only) problem with learning new things on this site is the time it takes to go through old code (or code written yesterday) to see if you made the mistake you didn't know at the time lol! The number of times this happens... — Adam Houldsworth, May 22 '12 at 15:31
@Adam - Haha true. I have 666 answers, so I let the evil old code live on :P — SwDevMan81, May 22 '12 at 15:40
@AdamHouldsworth: I prefer to keep the event delegate around as a matter of habit, since there isn't always an obvious visual difference between anonymous delegates that create a single static delegate instance, create an instance which is bound to the object creating them, or create an instance which is bound to a closure; even though creating a new delegate from a named instance method is guaranteed to yield one that will compare equal to any other delegate created from that same method on that same instance, it still doesn't feel right. — supercat, May 22 '12 at 15:40
@supercat That's a good shout. It also improves expressing the intent of the code for future maintenance - someone can come along and see (and search on) delegate references. — Adam Houldsworth, May 22 '12 at 15:42

Difference between two dispose implementations?

1 Answers1