views:

2216

answers:

5

We're all familiar with the horror that is C# event declaration. To ensure thread-safety, the standard is to write something like this:

public event EventHandler SomethingHappened;
protected virtual void OnSomethingHappened(EventArgs e)
{            
    var handler = SomethingHappened;
    if (handler != null)
        handler(this, e);
}

Recently in some other question on this board (which I can't find now), someone pointed out that extension methods could be used nicely in this scenario. Here's one way to do it:

static public class EventExtensions
{
    static public void RaiseEvent(this EventHandler @event, object sender, EventArgs e)
    {
        var handler = @event;
        if (handler != null)
            handler(sender, e);
    }
    static public void RaiseEvent<T>(this EventHandler<T> @event, object sender, T e)
        where T : EventArgs
    {
        var handler = @event;
        if (handler != null)
            handler(sender, e);
    }
}

With these extension methods in place, all you need to declare and raise an event is something like this:

public event EventHandler SomethingHappened;

void SomeMethod()
{
    this.SomethingHappened.RaiseEvent(this, EventArgs.Empty);
}

My question: Is this a good idea? Are we missing anything by not having the standard On method? (One thing I notice is that it doesn't work with events that have explicit add/remove code.)

+8  A: 

It will still work with events that have an explicit add/remove - you just need to use the delegate variable (or however you've stored the delegate) instead of the event name.

However, there's an easier way to make it thread-safe - initialize it with a no-op handler:

public event EventHandler SomethingHappened = delegate {};

The performance hit of calling an extra delegate will be negligible, and it sure makes the code easier.

By the way, in your extension method you don't need an extra local variable - you could just do:

static public void RaiseEvent(this EventHandler @event, object sender, EventArgs e)
{
    if (@event != null)
        @event(sender, e);
}

static public void RaiseEvent<T>(this EventHandler<T> @event, object sender, T e)
    where T : EventArgs
{
    if (@event != null)
        @event(sender, e);
}

Personally I wouldn't use a keyword as a parameter name, but it doesn't really change the calling side at all, so do what you want :)

EDIT: As for the "OnXXX" method: are you planning on your classes being derived from? In my view, most classes should be sealed. If you do, do you want those derived classes to be able to raise the event? If the answer to either of these questions is "no" then don't bother. If the answer to both is "yes" then do :)

Jon Skeet
Good point; @event can't change once you're in the method. I was aware that you can subscribe an empty delegate, but my concern was more about whether it's good or bad to dispense with the On method.
Kyralessa
I recognize this suggestion to use delegate{} from your excellent book!This is awesome :)
hmemcpy
Just remember if your class sets the delegate to null, you'll still have an error. I really don't understand why so many people have a problem with this.
Robert Paulson
You're right, Jon; I see now that the add/remove wasn't the problem. The problem was that the event was in a base class. I was testing in Form1 on an event declared in Form.
Kyralessa
@Jon Skeet: I just tested this on an event that didn't use an explicit add/remove and it still worked correctly.
Scott Dorman
@Scott: I don't think anyone suggested that it wouldn't, did they?
Jon Skeet
@Jon Skeet: Sorry...mis-read your first sentence and somehow thought the "still" meant "only".
Scott Dorman
You guys need to add the [MethodImpl(MethodImplOptions.NoInlining)] attribute to these extension methods or else your attempt to copy the delegate to a temporary variable could be optimized away by the JITter, allowing for a null reference exception. This could occur in both Kyralessa's and Jon Skeet's versions. So just add [MethodImpl(MethodImplOptions.NoInlining)] and you are fine, and, yes, you would no longer need to explicitly copy to a temporary variable since simply passing in the value via the method's parameter accomplishes this.
Mike Rosenblum
Can there really be a race condition when the method is not marked as NoInlining? See this question http://stackoverflow.com/questions/2327343/can-method-inlining-optimization-cause-race-conditions
SelflessCoder
@Mike: No, I don't believe the JITter *is* allowed to do that. This was raised as an issue a while back, but I've spoken with someone (I can't remember whether it was Joe Duffy or Vance Morrison; someone like that) who said that was completely against the rules of what's allowed by the JITter and the memory model.
Jon Skeet
Interesting point Jon, it looks like it is probably necessary. My source was Juval Lowy, but I replied further to your answer here: http://stackoverflow.com/questions/2327343/can-method-inlining-optimization-cause-race-conditions
Mike Rosenblum
A: 

Less code, more readable. Me like.

If you're not interested in performance you can declare your event like this to avoid the null check:

public event EventHandler SomethingHappened = delegate{};
Cristian Libardo
+1  A: 

You're not "ensuring" thread safety by assigning the handler to a local variable. Your method could still be interrupted after the assignment. If for example the class that used to listen for the event gets disposed during the interruption, you're calling a method in a disposed class.

You're saving yourself from a null reference exception, but there are easier ways to do that, as Jon Skeet and cristianlibardo pointed out in their answers.

Another thing is that for non-sealed classes, the OnFoo method should be virtual which I don't think is possible with extension methods.

Isak Savo
I think they meant to say 'avoiding a race condition'.
Robert Paulson
+1  A: 

[Here's a thought]

Just write the code once in the recommended way and be done with it. Then you won't confuse your colleagues looking over the code thinking you did something wrong?

[I read more posts trying to find ways around writing an event handler than I ever spend writing an event handler.]

Robert Paulson
A: 

The funny thing about this IMHO is that, after following Jon Skeet's simplification of your proposal, the extension method looks like this:

static public void RaiseEvent(this EventHandler @event, object sender, EventArgs e)
{
    @event(sender, e);
}

So you're going to replace one line of code, namely

SomethingHappened(this, new EventArgs(...));

by another line of code, namely:

SomethingHappend.RaiseEvent(this, new EventArgs(...));

which adds ".RaiseEvent".Length characters, plus the extension method definition, a using statement and possibly a reference to the assembly containing the extension method.

Am I wrong?

chiccodoro