tags:

views:

546

answers:

9

Some text before the code so that the question summary isn't mangled.

class Tree
{
    public event EventHandler MadeSound;

    public void Fall() { MadeSound(this, new EventArgs()); }

    static void Main(string[] args)
    {
     Tree oaky = new Tree();
     oaky.Fall();
    }
}

I haven't used events much in C#, but the fact that this would cause a NullRefEx seems weird. The EventHandler reference is considered null because it currently has no subsribers - but that doesn't mean that the event hasn't occurred, does it?

EventHandlers are differentiated from standard delegates by the event keyword. Why didn't the language designers set them up to fire silently in to the void when they have no subscribers? (I gather you can do this manually by explicitly adding an empty delegate).

+6  A: 

Well, the canonical form is:

void OnMadeSound()
{
    if (MadeSound != null)
    {
        MadeSound(this, new EventArgs());
    }
}

public void Fall() {  OnMadeSound(); }

which is very slightly faster that calling an empty delegate, so speed won out over programming convenience.

James Curran
I think it's recommended that the On* method is protected to allow a derived class to react to the event or to change how or when the event is called. Just as an addition.
OregonGhost
Actually, the safest form is this:protected void OnMadeSound() { EventHandler tempHandler = MadeSound; if (tempHandler != null) { tempHandler(this, new EventArgs()); }}
Scott Dorman
The method should be protected and the tempHandler prevents the possibility that a NullReferenceException occurs if the handler is deleted between the null check and actually raising the event.
Scott Dorman
+3  A: 

Very Zen, eh?

You have to test for null when you want to raise an event:

protected void OnMyEvent()
{
    if (this.MyEvent != null) this.MyEvent(this, EventArgs.Empty);
}

It would be nice if you didn't have to bother with this, but them's the breaks.

Jon B
Cool, EventArgs.Empty :)
frou
+2  A: 

James provided a good technical reasoning, I would also like to add that I have seen people use this an advantage, if no subscribers are listening to an event, they will take action to log it in the code or something similar. A simpl example, but fitting in this context.

Mitchel Sellers
+3  A: 

You need to understand what your event declaration is actually doing. It's declaring both an event and a variable, When you refer to it within the class, you're just referring to the variable, which will be null when there are no subscribers.

Jon Skeet
+2  A: 

What is the point of raising an event if no one is listening? Technically, its just how C# chose to implement it.

In C#, an event is a delegate with some special feathers. A delegate in this case can be viewed as a linked list of function pointers (to handler methods of subscribers). When you 'fire the event' each function pointer is invoked in turn. Initially the delegate is a null object like anything else. When you do a += for the first subscribe action, Delegate.Combine is called which instantiates the list. (Calling null.Invoke() throws the null exception - when the event is fired.)

If you still feel that "it must not be", use a helper class EventsHelper as mentioned here with old and improved 'defensive event publishing' http://weblogs.asp.net/rosherove/articles/DefensiveEventPublishing.aspx

Gishu
+4  A: 

Another good way I've seen to get around this, without having to remember to check for null:

class Tree
{
    public event EventHandler MadeSound = delegate {};

    public void Fall() { MadeSound(this, new EventArgs()); }

    static void Main(string[] args)
    {
        Tree oaky = new Tree();
        oaky.Fall();
    }
}

Note the anonymous delegate - probably a slight performance hit, so you have to figure out which method (check for null, or empty delegate) works best in your situation.

Chris Marasti-Georg
your code can still set the event MadeSound to null, so you can still get a NullReferenceException, making the delegate{} pointless
Robert Paulson
Or, instead of setting it to null, you could set it to delegate {}... How often do you set an event to null? I haven't ever had to yet.
Chris Marasti-Georg
A: 

Thank you for the responses. I do understand why the NullReferenceException happens and how to get around it.

Gishu said

What is the point of raising an event if no one is listening?

Well, maybe it's a terminology thing. The appeal of an "event" system seems to me that all the responsibility of the fallout of the event that took place should be on the watchers and not the performer.


Perhaps a better thing to ask is: If a delegate field is declared with the event keyword in front of it, why doesn't the compiler translate all instances of:

MadeSound(this, EventArgs.Empty)

to

if (MadeSound != null) { MadeSound(this, EventArgs.Empty); }

behind the scenes in the same manner that other syntax shortcuts are? The number of boilerplate OnSomeEvent null checking methods that people have to write manually must be colossal.

frou
The compiler's isolating what it's doing here into a single aspect - creating a variable and an event with the same name. I'd rather it didn't do more than that, for the sake of complexity. Avoiding the null checking is easy though: public event EventHandler Click = delegate{};
Jon Skeet
So from outside the class, MadeSound refers to an "event", which manages adding/removing suitable delegates to an instance of the EventHandler delegate that shares the same identifier and hides the "event" internally? Meaning the event isn't directly fired, but rather the delegate list it produced?
frou
@Jon I disagree. I have a lot of respect for you but fail to see why you'd advocate the delegate{} initializer pattern.
Robert Paulson
+2  A: 

The recommended pattern is (.net 2.0+)

public class MyClass
{
    public event EventHandler<EventArgs> MyEvent; // the event

    // protected to allow subclasses to override what happens when event raised.
    protected virtual void OnMyEvent(object sender, EventArgs e)
    {
        // prevent race condition by copying reference locally
        EventHandler<EventArgs> localHandler = MyEvent;
        if (localHandler != null)
        {
            localHandler(sender, e);
        }
    }
    public void SomethingThatGeneratesEvent()
    {
        OnMyEvent(this, EventArgs.Empty);
    }
}

I see a lot of recommendations for an empty delegate{} in an initializer, but I totally disagree with it. If you follow the above pattern you only check the event != null in one place. The empty delegate{} initializer is a waste because it's an extra call per event, it wastes memory, and it still can fail if MyEvent was set to null elsewhere in my class.

* If your class is sealed, you wouldn't make OnMyEvent() virtual.

Robert Paulson
+1  A: 

Using an extension method would be helpful in this scenario.

public static class EventExtension
{
    public static void RaiseEvent<T>(this EventHandler<T> handler, object obj, T args) where T : EventArgs
    {
        if (handler != null)
        {
            handler(obj, args);
        }
    }
}

It can then be used like below.

public event EventHandler<YourEventArgs> YourEvent;
...
YourEvent.RaiseEvent(this, new YourEventArgs());
Taylor Leese
Thanks. I actually came to that approach myself and made one for plain old EventArgs, too :)
frou