OK, sometimes you have an idea that is one point (seemingly) brilliant, simple, and kind of stupid all in one shot. This is one of those. Actually, I’m still trying to figure out if this is really a good idea or not — and what better way to do that than to post the idea on a blog and strike a flame war! Bring it on.
Lets start with the standard model for creating and firing events.
Step 1: Create the event
public eventEventHandler<EventArgs> OnLoadData;
My current method is to always use the generic EventHandler<> instead of the old-school EventHandler. I think it makes for better-looking code IMHO.
Step 2: Fire the Event (Old Skool)
1: if(OnLoadData != null) 2: { 3: OnLoadData(this, null); 4: }
Step 3: We can do better
There is a cool feature that you get with extension methods: you can call an extension method on null base object! So you can now do things that were impossible — like removing a null check when firing an event and just fire the event!
Step 4: Create an extension method
To start you have to create a static class and … oh forget it, here is the code.
1: using System; 2: 3: namespaceMyNamespace 4: { 5: public static classMyExtensions 6: { 7: public static voidFire<TEventArgs>( thisEventHandler<TEventArgs> myEvent, objectsender, TEventArgs e) whereTEventArgs: EventArgs 8: { 9: if(myEvent != null) 10: myEvent(sender, e); 11: } 12: } 13: }
Step 5: Refactor Step 2
1: OnLoadData.Fire(this, null);
Done.
Step 6: Profit
Well, I will as soon as I figure out how. I mean really, I save 3 friggen lines of code here. Big deal–right? Well, There are some moral victories in this.
- I simplified the firing of events ever so slightly by removing previously mandatory logic.
- It is reusable. The Fire extension method can be used with any event that we create with the generic EventHandler.
- I have added an ever so small amount of clarity to what the code is doing. Granted, this will only clear things up for the extreme noob or clueless manager who wants to see the code…but still. Let’s not be snobby about this.
- I figured out a grand way to use and abuse Extension Methods in a useful way. It makes me feel special. What can I say.
- And I like it. Something about always having to check if the EventHander was null -EVERY SINGLE TIME I WANTED TO JUST FIRE A FREEKING EVENT- bugged me for some inexplicable reason. This little hack sets my soul at ease. So there.
That is sweet.
I am definitely going to give this a try as a means to remove duplicate code in my event handlers. I am with you in that I should not have to check for nulls with each event.
On another note, not sure if you do this as a practice or not, but there can exist a race condition between the time the “EventHandler myEvent” is created and tested for null. If another thread removes the handler, it will crash. I read an article on this by Jay Bazuzi and Eric Gunerson, and to prevent this, you could do the following.
EventHandler eventHanlder = myEvent;
if(eventHanlder != null)
{
eventHanlder(sender, e);
}
This is nice.
Though what you really should be doing is first name your events without the On- prefix, like DataLoading or DataLoaded depending on if it is invoked before or after the load. Then have a protected virtual void OnDataLoaded(EventArgs e) which invokes the event with “this” as the sender and the given event args. Now subclasses can intercept your event invocation, and your internal callsites are simplified to OnDataLoaded(EventArgs.Empty) or similar.
Also note what Alex just said about the race condition. The code he shows is actually what the code snippet “invoke” will give you in VS.
A different thing the extension-method-works-on-null-objects would be nice for is string.IsNullOrEmpty(myString) => myString.IsNullOrEmpty().
Neat…
Events are raised, not fired…
Nice!
@Alex: There is no race condition here. When you invoke am event directly in your code:
if (SomeEvent != null)
SomeEvent(this, …); // bad!
then someone might be changing it after the if, but before the event gets invoked. That’s what the invoke-snipped you posted prevents, by copying the current event handler into a local variable.
However, if you’re calling a method to fire your event (extension method or not) then the event handler is implicitly copied on the stack, unless it’s passed as a ref parameter. So Cris’ code is not only simpler, it is also thread-safe!
Nice, I like it. I’ve always been really bothered that the null check just can’t be built into the language and I’ve always had to build a similar static helper method in a helper class. I like this a lot better.
Nice job
Nifty.
FWIW, we did something similar in our .NET 2 codebase. We had a static class called DelegateInvoke, with a method to raise an event:
public static class DelegateInvoker
{
public static Raise(EventHandler handler, object sender, T eventArgs)
{
if(handler != null) handler(sender, eventArgs);
}
}
When then raised events like this:
DelegateInvoker.Raise(SomeEvent, sender, args);
That said, your use of extension methods makes for a more natural syntax.
I turns out that if dealing with a multi-threaded app, raising events is not so simple.
http://davebrooks.wordpress.com/2007/02/07/threading-through-the-open-window-of-opportunity/
but what benifit did it really get you ? saving you a few keystrokes ? for the price of future maintenability, it doesn’t justify the cost…
@Alexis Rzewski,
Maintainability and readability are always top of my list for any code I write.
I fail to see how this would not be maintainable, and I still think it is very readable.
So do you not like extension methods? Or is there something specific about my implementation that you don’t like?
You know, you could easily just define the event with an existing delegate so that you never have to check for null.
public event EventHandler<MyEventArgs> OnMyEvent = delegate {};