This is where 'Yort' praises, rants, raves and otherwise talks nonsense about Microsoft .NET and Visual Studio. Or anything else related to software development or Microsoft that he feels like throwing in. It's his blog after all...

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);

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 voidMyEventDelegate(object sender, MyEventArgs e);

public eventMyEventDelegateMyEvent;

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 voidMyEventDelegate(int myValue); public eventMyEventDelegateMyEvent;

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 EventArgsto 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.

The post was from the Yort On .NET Blog (http://www.yortondotnet.blogspot.com). The opinions here in are probably (but not neccesarily) those of Yort, but are certainly not those of any person, company, or organisation related or unrelated to Yort, unless otherwise stated.