0

I have this method that can be used as a delegate. Because of reasons, I want it to do some stuff only once for each time the event fires.

//In the real code those lines are in very different places
btn.Click += foo.DoOnlyOnce;
btn.Click += foo.DoOnlyOnce;

will result in only one execution of DoStuff()

 private List<EventArgs> _eventArgsList = new List<EventArgs>();

 public void DoOnlyOnce(object sender, EventArgs e)
 {            
     if (!_eventArgsList.Contains(e))
     {
          _eventArgsList.Add(e);
          DoStuff();
     }            
 }

If the btn is Clicked twice, it still does DoStuff() twice, as intended.

A problem with this solution is _eventArgsList it will keep track of the eventArgs indefinitely. A memoryleak is born since GC will not dispose it like it would normally do once all subscriptions are handled.

Can I somehow get the original event in the delegate?

For example something like

GetOriginalEvent().GetInvocationList()

Would help me to determinate when it is a good time to clear the list.

Or, can I solve this in a better way? I was originally starting to try something like:

var handler = new DoEventOnlyOnce(btn.Click);
handler.DoOnlyOnce += DoStuff;
handler.DoOnlyOnce += DoStuff;

So you could have a reference to the original event. But didn't get that working.

You could argue that I should always do 

btn.Click -= foo.DoOnlyOnce;
btn.Click += foo.DoOnlyOnce;

But I want this logic inside DoOnlyOnce so I don't have to rely on the correct usage.

Stijn Van Antwerpen
  • 1,840
  • 17
  • 42
  • 3
    So you really only want to attach the _handler_ once, right? Why is it getting added multiple times in the first place? Seems like _that_ is where your control should be. – D Stanley Sep 10 '15 at 14:22
  • 3
    Note that [the proper solution](http://stackoverflow.com/questions/2697247/how-to-determine-if-an-event-is-already-subscribed) is the one you seem reluctant to implement. – D Stanley Sep 10 '15 at 14:24
  • Legacy code... events are added and detached all over, sometimes in a non predicative order (since this also sometimes happens in events...). I wanted to solve this in a dedicated class. – Stijn Van Antwerpen Sep 10 '15 at 14:27
  • Until some handler will last for 6 minutes in a non async way :-) I don't like that kind of patches... Then I will prefer -= / += in every place were it is used. – Stijn Van Antwerpen Sep 10 '15 at 14:37
  • 1
    @StijnVanAntwerpen as Stanley noted, the answer for your question: "Can I somehow get the original event in the delegate?" is "No, main purpose of events is to prevent you from doing that" – netaholic Sep 10 '15 at 14:40
  • 1
    Note that if the caller uses `EventArgs.Empty` then your check will fail since it is a static instance. – D Stanley Sep 10 '15 at 14:41
  • There is no reasonable way to solve the problem in specifically the way you are asking. There are only hacks, any of which are problematic and in any case leave your original bugs in place. The right way to fix this is to bite the bullet and go fix the "legacy code" that is broken. Just because it's legacy code, that doesn't mean it's not worth fixing bugs in it. In correctly-written code, event subscription is handled in a well-organized way that avoids such problems. – Peter Duniho Sep 10 '15 at 18:27
  • You don't need to store all `EventArgs`, just the last one. Change `List` to `EventArgs`. This significantly reduces the leak. It's still a hack, though. – Henrik Sep 11 '15 at 06:56
  • @Henrik: I did not mention it, but... multithreading, that is why I keep a list (did also not include the lock in the example). It might also happen that one event triggers another, which will be first handled before the second handler of the first event will be executed... – Stijn Van Antwerpen Sep 11 '15 at 07:24

1 Answers1

0

How about using a simple boolean switch:

private bool _Executed;

private void OnButtonClick(object sender, EventArgs e)
{
    if(!_Executed)
    {
        _Executed = true;
        DoOnlyOnce();
    }    
}

private void DoOnlyOnce()
{
    Console.WriteLine("Watch carefully. You'll never see me again.");
}

I put the logic within the event handler and outside of the DoOnlyOnce() method. But I think it is just a matter of taste (or other logic in your real code) if you like it to be within that method or outside within the calling code.

Okay, after getting the comments and re-reading your question, let's do another approach. We create a cache where we remember who is currently running (the sender and how far we got Task). With that it should be possible to solve that issue:

private Dictionary<Object, Task> _Workers = new Dictionary<object, Task>();

private void OnButton1Click(object sender, EventArgs e)
{
    StartIfNotBusy(sender);
}

private void OnButton2Click(object sender, EventArgs e)
{
    StartIfNotBusy(sender);
}

private void StartIfNotBusy(object sender)
{
    Task worker;

    if (_Workers.TryGetValue(sender, out worker)
        && !worker.IsCompleted)
    {
        Console.WriteLine("Sorry, I'm currently busy.");
        return;
    }

    _Workers[sender] = DoSomething();
}

private Task DoSomething()
{
    return Task.Delay(1000).ContinueWith(_ => Console.WriteLine("Hey, I finished working."));
}
Oliver
  • 43,366
  • 8
  • 94
  • 151