Wednesday, February 28, 2007

Creating and Raising Events - The Right Way

Editors Note : There is a follow up post to this one with critical information. If you read this article, please read the follow up as well.

I've seen quite a few people new to C# who haven't grasped the correct/best way to declare and raise events, particularly when writing objects you expect other developers to inherit from.

The most common mistake I see made is pre-fixing the word On to event names, i.e OnProgressChanged.

This is a no-no because because every event (at least in a class that may be inherited from) should have a protected method with that name (i.e OnEventName). For example, if you wanted to have an event that occurred when progress of a long-running operation changed, you'd declare the following (or something like it);

public event EventHandler<ProgressChangedEventArgs> ProgressChanged;

protected virtual OnProgressChanged(ProgressChangedEventArgs e)
{
if (ProgressChanged != null)
ProgressChanged(e);
}

Editors Note : The above sample code has a race condition in it. See the follow up post for the correct way to do this.

The reason for the protected On method is to allow class deriving from this one to raise the event. In .NET you can only raise an event from within the class that declared it, so if you tried to say

if (ProgressChanged != null)
ProgressChanged(e);

in a derived class, you would receive a compile error. The reason for this is that good design dictates only the object that owns the event should know when to raise it, and so only it should be able to. However, since code in derived classes are really part of the object they should also be allowed this privilege. By providing this method to raise the event, and making it protected, we ensure that only the root class, and any derived classes but no external code, can raise the event.

The virtual keyword in the On method declaration is not required, but I prefer to use it as it allows derived classes to know when the event is about to be raised and either prevent it, alter the event arguments, or raise other events/execute other code before or after the event is raised.

Another, less common problem I've seen is code that fails to check if the event delegate is null before invoking it. In .NET you raise an event by invoking a multi-cast delegate, which is a fancy way of saying you just write the event name, i.e

DisplayProgress(e);

will invoke the DisplayProgress event - BUT- if there is no one connected to the event yet a null reference exception will occur, which is why you should always put this first;

if (EventName != null)

Even if you wanted an exception thrown when no one was connected to your event, I would suggest throwing a custom exception with a more meaningful message, rather than allowing the generic null reference exception to be thrown.

Finally, I often see code that explicitly defines a delegate for each event. For example;

public delegate void MyEventDelegate(object sender, MyEventArgs e);

public event MyEventDelegate MyEvent;

This is fine, and there is nothing inherently wrong with it, but Microsoft have (thankfully) provided us with a shortcut, using a generic delegate for all events. The above code can be simplified to;

public event EventHandler<MyEventArgs> MyEvent;

This is not only less typing, but means there's fewer declarations floating around polluting your intellisense/namespaces.

One last thing, some times I've seen code that declares events using delegates that don't conform to normal event handler signatures, i.e

public delegate void MyEventDelegate(int myValue);
public event MyEventDelegate MyEvent;

The problem here is the delegate used doesn't have a sender parameter (declared as an object), nor does it use an object inheriting from EventArgs to provide the myValue integer value to the event handlers.

This is bad practice. All events in the .NET framework and associated libraries (or at least all the ones I've seen) conform to the object sender, eventargs e syntax and it's going to be expected from most people coding against your events. Also, by not using an object inheriting from the EventArgs class to pass your event parameters you're going to have to do a lot more refactoring if you want to add or remove parameters (and any third party code written against the old signature will need to refactoring too). Finally, you haven't provided a reference to the object raising the event, so if someone has a single event handler receiving event notifications from multiple sources, they'll have no idea where the event came from.

The only time it *might* be acceptable to break from this standard is when you don't want to pass any parameters at all, and are 100% sure you never will. In which case it might be ok to pass only a reference to the sender and no EventArgs. I must admit that having to create/receive an instance EventArgs always seems fairly pointless and stupid to me, but the benefit is (again) that you don't have to refactor so much if you add parameters later.

3 comments:

  1. Thanks for that nice little tutorial. It really helped me.

    ReplyDelete
  2. No problem Stephen, always nice to hear when you've helped someone out :)

    ReplyDelete
  3. Hi Stephen,

    Please see my follow up post at; http://yortondotnet.blogspot.com/2009/07/follow-up-to-creating-and-raising.html which has important information in it.

    ReplyDelete