Is it OK to always add an empty event handler?

Question

Possible Duplicate:
Is there a downside to adding an anonymous empty delegate on event declaration?

The following pattern is quite common when using event handlers (in C#):

public event Action handler;
…
// some method:
if(handler != null) handler();

Are there any downsides of assigning an empty delegate to this event? This would save the if !=null condition everywhere, where the event is fired. Of course, this only applies, when the we cannot guarantee that the event is always assigned a proper delegate.

public event Action handler;
…
// in constructor:
handler += ()=>{};
…
// some method:
handler();

Sure, there's a slight performance hit, but it makes the code much cleaner. What's the best practice in such a case? Any technical disadvantages?

From my point of view isn't a good practice. Yes, you save a null check, but, a few months later, when inspecting the code for bugs or maintenance, the first example better demonstrates the purpose of the code. — Steve, May 24 '12 at 11:44
It gets even worse - without protection, the `handler` may be set to null between checking for null and invoking it. You should copy the value of `handler` into a local variable and check _that_ for null. — C.Evenhuis, May 24 '12 at 11:46
Your first code is broken since you don't copy the value of `handler` into a local variable. — CodesInChaos, May 24 '12 at 11:47
@CodeInChaos: why do I need to copy it into a local variable in the first example? — knittl, May 24 '12 at 11:53
Because event subscription/unsubscription should be threadsafe. Your code can throw a `NullReferenceException` if the last handler unsubscribes between your check and raising the event. — CodesInChaos, May 24 '12 at 11:54
@knittl if you are running this event from another thread the subscriber to the event may un-subscribe between checking for null and the event being run, which would then cause the handler to be null again. copying to a local variable will keep the state of the handler at that time and avoid un-registration errors. — Skintkingle, May 24 '12 at 11:55

david.s · Answer 1 · 2012-05-24T12:18:54.267

1

Instead of adding an empty delegate in the constructor, you can wrap the handler in a function which first checks if the handler is null then calls it. The downside of this is if you have a lot of events, you will have a lot of functions that wrap each event.

private void HandlerWrapper()
{
    Action localHandler = handler;
    if (localHandler != null) handler();
}

edited May 24 '12 at 12:18

answered May 24 '12 at 11:44

david.s

11,283
6
50
82

1

same thread safety issues as the original code – CodesInChaos May 24 '12 at 11:47

Skintkingle · Answer 2 · 2012-05-24T12:01:28.083

Interesting idea, I've never thought of doing that. The way i do my custom events is i make a OnCustomEventName taking parameters for the event and just do the check in there for null. and call OnCustomEventName from the code wherever i want the event triggered from. Avoids any performance hits and keeps the operational code cleaner than a 2-line if check every time you want the event fired.

That being said this isn't answering the question about technical disadvantages but more of a best practice when firing events.

example code for threadsafe "On" function.

private void OnCustomEventName()
{
    DelegateName localhandler = CustomEventName;
    if (localhandler != null)
        localhandler();
}

score 0 · Answer 3 · answered May 24 '12 at 11:50

I haven't really found any big downsides to do this and generally I prefer it over checking for null. The only issue I cant think of is that it might lead to annoying steps in the code when debugging (i.e. having to step over the empty delegate whenever stepping into an event).

I think that performance isn't an issue - if application performance degenerates significantly by invoking events, the event should probably not have been there in the first place.

Is it OK to always add an empty event handler?

3 Answers3