58

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?

quetzalcoatl
  • 32,194
  • 8
  • 68
  • 107
Judah Gabriel Himango
  • 58,906
  • 38
  • 158
  • 212
  • Better question: useful or not? – Jason Bunting Oct 29 '08 at 19:21
  • 8
    It's useful. Instead of littering hundreds of "if (SomeEvent != null) SomeEvent(this, args)" around our codebase, we can replace it with a single line. – Judah Gabriel Himango Oct 29 '08 at 19:22
  • Or, you could just create a 'dummy handler' for your event, to ensure it's never null. – Joel Coehoorn Oct 29 '08 at 19:23
  • Joel - what are the performance implications of dummy handlers? I haven't done the proofing on it but I'm willing to bet that a conditional check is less expensive than a delegate invocation. – Erik Forbes Oct 29 '08 at 19:28
  • Interesting suggestion about initializing them to an empty delegate. Seems like that would create some overhead in terms of both generated code and runtime memory. – Judah Gabriel Himango Oct 29 '08 at 19:36
  • Personally I like the idea of adding an empty delegate better: http://stackoverflow.com/questions/170907/is-there-a-downside-to-adding-an-anonymous-empty-delegate-on-event-declaration Still a neat idea though :) +1 – Luke Quinane Nov 16 '08 at 07:17
  • I think the code is fine, although I should point out that the code in its current form isn't thread safe. – Juliet Mar 22 '09 at 19:46
  • If it's inlined it solves the thread-safety issue which requires yet another line of boilerplate if you don't use something like this – Mark Sowul Apr 07 '11 at 12:55
  • Old question, but under current "rules" it should really be closed. Still interesting to read though. – JDB May 16 '13 at 18:43

8 Answers8

35

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

Dan Goldstein
  • 23,436
  • 10
  • 47
  • 51
  • 2
    I agree events without subscribers shouldn't be null. It seems rather foolish that an event with no subscribers is null - surely they could have initialized it with some reusable class with an empty subscription list. Alas, it's too late to change now. – Judah Gabriel Himango Oct 29 '08 at 19:26
  • I entirely agree, and have an overload that defaults the args argument to EventArgs.Empty. – kͩeͣmͮpͥ ͩ Mar 07 '11 at 16:49
  • 1
    Having events without subscribers be null is just fine. What's broken is that trying to invoke a null event which has a void return type throws an exception. Invoking a null delegate should throw an exception, but a field declared *as an event* should not be regarded as a delegate in such case. – supercat Nov 18 '11 at 00:05
14

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

jonnii
  • 28,019
  • 8
  • 80
  • 108
9

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

alvin
  • 179
  • 1
  • 2
6

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);
        }
    }
Squirrel
  • 1,355
  • 2
  • 16
  • 25
5

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.

David
  • 3,177
  • 1
  • 18
  • 15
  • It would be evil only because it won't throw an exception if MyButtonClick is null. E.g. this is valid: EventHandler click = null; click.Raise(...); – Judah Gabriel Himango Oct 29 '08 at 19:32
  • Well you can't set the EventHandler null. The only way it can be null is if no one is listening to the event, and I don't think raising an event that no one is listening to is an exception-worthy event. – David Oct 29 '08 at 19:56
  • Right, I just mean, if there are no subscribers to the event, it would be null. Then, calling what looks like a instance method on a potentially null object might appear to be an evil use of extension methods. – Judah Gabriel Himango Nov 03 '08 at 18:45
0

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.

Bob
  • 5,510
  • 9
  • 48
  • 80
  • 2
    Yep, true. I'm willing to trade a little memory allocation for more readable, robust code. – Judah Gabriel Himango Feb 18 '10 at 15:09
  • 2
    The overhead is minimal and if you look at how all the core lbraries are done they implement the following pattern: public void SomeMethod() { OnSomeEvent(new SomeEventArgs()); } protected virtual void OnSomeEvent(SomeEventArgs e) { if (SomeEvent != null) { SomeEvent(this, e); } } – Bronumski Oct 02 '10 at 07:40
0

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?

Greg D
  • 43,259
  • 14
  • 84
  • 117
-2

Throwing an exception when there are no handlers is not really preferable by the most. If it does not have an handlers it is better to be empty rather than null.

Prakash
  • 823
  • 12
  • 28
  • @MattEllen Oops, I thought it will help in knowing the general practice. Considering the programmer comfort. Thanks anyway – Prakash Sep 13 '11 at 07:20