Evil use of extension methods?

Question

Evil or not evil?

public static void Raise(this EventHandler handler, object sender, EventArgs args)
{
   if (handler != null)
   {
      handler(sender, args);
   }
}

// Usage:
MyButtonClicked.Raise(this, EventArgs.Empty);

// This works too! Evil?
EventHandler handler = null;
handler.Raise(this, EVentArgs.Empty);

Note that due to the nature of extension methods, MyButtonClicked.Raise will not throw a NullReferenceException if MyButtonClicked is null. (E.g. there are no listeners to MyButtonClicked event).

Evil or not?

Answer 1

Not evil. I wish events worked this way by default. Can someone explain why an event with no subscribers is null?

Answer 2

Why would it be evil?

Its purpose is clear: It raises the MyButtonClicked event.

It does add a function call overhead, but in .NET it will either be optimized away or pretty fast anyway.

It is slightly trivial, but it fixes my biggest complaint with C#.

On the whole, I think it's a fantastic idea, and will probably steal it.

Answer 3

You can always declare your events like this (not that i recommend it):

public event EventHandler<EventArgs> OnClicked = delegate { };

That way they have something assigned to them when you call them, so they don't throw a null pointer exception.

You can probably get rid of the delegate keyword in C# 3.0...

Answer 4

I wouldn't say it's evil, but I'm interested in how your extension method fits in with the

protected virtual OnSomeEvent(EventArgs e){ }

pattern and how it handles extensibility via inheritance. Does it presume all subclasses will handle the event instead of override a method?

Answer 5

Coming from a java background this has always seemed odd to me. I think that no one listening to an event is perfectly valid. Especially when listeners are added and removed dynamically.

To me this seems one of C#'s gottchas that causes bugs when people don't know / forget to check for null every time.

Hiding this implementation detail seems a good plan as it's not helping readability to check for nulls every single time. I'm sure the MSFTs will say there's a performance gain in not constucting the event if no one is listening, but imho it is vastly outweighed by the pointless null pointer exceptions / reduction in readability in most business code.

I'd also add these two methods to the class:

    public static void Raise(this EventHandler handler, object sender)
    {
        Raise(handler, sender, EventArgs.Empty);
    }

    public static void Raise<TA>(this EventHandler<TA> handler, object sender, TA args)
        where TA : EventArgs
    {
        if (handler != null)
        {
            handler(sender, args);
        }
    }

Answer 6

Don't forget to use [MethodImpl(MethodImplOptions.NoInlining)], else its possible that it isn't thread safe.

(Read that somewhere long ago, remembered it, googled and found http://blog.quantumbitdesigns.com/tag/events/ )

Answer 7

Although I wouldn't describ it as evil, it still has a negative implication, as it adds unnecessary overhead:

When calling

myEvent.Raise(this, new EventArgs());

the object EventArgs is initialized in all situations, even if no-one subscribed to myEvent.

When using

if (myEvent!= null) {
   myEvent(this, new EventArgs());
}

EventArgs is only initialized if someone subscribed to myEvent.