26

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

Is it a good practice to define an empty delegate body for a event so that you do not need to worry raise a event which have no event handler? ( no need to check whether event is null).

Like code below:

public event EventHandler<LoadEventArgs> LoadedData = delegate { };
Community
  • 1
  • 1
Kurt Liu
  • 610
  • 2
  • 8
  • 15
  • 1
    Not everybody is too happy about this, for details check here: http://stackoverflow.com/questions/9033/hidden-features-of-c/9282#9282 – gjvdkamp Nov 29 '10 at 11:45

3 Answers3

43

I've certainly found it useful, yes. There will be a tiny, tiny performance cost - but the benefit in readability for not having to perform the nullity test makes it worth it IMO.

It's worth pointing out that this is one of the few times when it's good to use an anonymous method rather than a lambda expression - otherwise you have to name the parameters that you're going to ignore, like this:

public event EventHandler<LoadEventArgs> LoadedData = (sender, args) => {};

I don't like having to name things I'm not intending to use :)

Jon Skeet
  • 1,421,763
  • 867
  • 9,128
  • 9,194
  • 3
    well, you can cay `(_,__) => {}` which is not too bad than `delegate {}` – Earth Engine Oct 02 '14 at 05:52
  • 4
    There is now a ?. operator that can be used instead of performing null checks, e.g. MyEvent?.Invoke(); It can be a useful alternative. – person27 Apr 22 '17 at 19:49
  • 2
    @Anon234_4521: Indeed, as per https://codeblog.jonskeet.uk/2015/01/30/clean-event-handlers-invocation-with-c-6/ - but I'm not going to update all my posts from several years ago :) – Jon Skeet Apr 22 '17 at 19:52
5

I would not do this.

There are two reasons for this. Firstly, it's standard to simply check the handler for null before calling this. You see this all over the place, in examples, in the Microsoft reference source and in the manuals.

Second, initializing your events this way allocates unnecessary objects when creating your class. This will put extra memory pressure on your application without a real need. This is because:

public event EventHandler<LoadEventArgs> LoadedData = delegate { };

translates to:

public event EventHandler<LoadEventArgs> LoadedData = new EventHandler<LoadEventArgs>delegate { });

If you don't do this already, wrapping your events in specific methods helps a lot:

public event EventHandler MyEvent;

protected virtual void OnMyEvent(EventArgs e)
{
    if (MyEvent != null)
        MyEvent(this, e);
}

You can make this thread safe(r) using the following:

public event EventHandler MyEvent;

protected virtual void OnMyEvent(EventArgs e)
{
    var handler = MyEvent;

    if (handler != null)
        handler(this, e);
}
Pieter van Ginkel
  • 29,160
  • 8
  • 71
  • 111
  • 7
    It will create a single extra object which will be shared by all instances, because the delegate doesn't access any captured variables. (Not guaranteed, but the MS compiler will do this.) Is that going to be significant? I doubt it. However, the chance of someone writing the non-thread-safe version (as per your first code snippet) is quite significant, IMO. Whether they'll actually end up with a NullReferenceException from it or not, who knows... but you can bet it'll happen at a customer site instead of in test... is the tiny performance degredation really worth the extra code? – Jon Skeet Nov 29 '10 at 11:45
  • 1
    @Jon Skeet - That depends. Firstly, the reason I've added the second snippet is specifically for that reason (think I've got that code from an earlier reply from you :) ). However, more often than not, classes aren't thread safe to begin with. Take for example `Dictionary<,>`. Don't try to add and read at the same time :). Concerning the new `delegate`, you're right about that. However, a new `EventHandler` is created for every event. `MyEvent = delegate {}` translates to `MyEvent = new EventHandler(delegate {})`. – Pieter van Ginkel Nov 29 '10 at 11:52
  • Each event will create one extra object, yes. Across the whole of the lifetime of the app, I think I can handle "wasting" a few hundred bytes for several events. Note that even if you *don't* care about thread-safety, using the "no-op handler" trick makes the event-raising code simpler... and it's one less thing to think about if you ever want to make the code thread-safe. – Jon Skeet Nov 29 '10 at 11:56
  • @Jon Skeet - Agree. However, you still have the case where you're not just creating the object once for the application, but you're creating many instances of that object. If you have a class that has 10 events and you create it 1000 times, you now have 11000 instances instead of 1000. – Pieter van Ginkel Nov 29 '10 at 12:22
  • 3
    No, you've missed the point of my first comment. While it's not guaranteed by the C# spec, the MS compiler at least will only create *one* instance of the delegate. Try it - you'll find a static field called something like `CS$<>9__CachedAnonymousMethodDelegate1` - and the constructor will populate that if necessary, then copy the reference to the event handler field. – Jon Skeet Nov 29 '10 at 12:32
  • A bigger factor than the extra delegate that gets created by the dummy subscription itself is the fact that using Delegate.Combine to combine a delegate and a null reference is much more efficient than using it to combine two delegates. Of course, the right thing would have been for the C# standard to specify that invocation of a thing declared as an "event" with a void return type will be a no-op when the event in question is null. If something is declared as a delegate, trying to invoke null should trigger a NullReferenceException, but empty event lists are a normal situation. – supercat Nov 17 '11 at 23:22
  • 5
    "Hardware is cheap, programmers are expensive." "Write for maintainability, refactor for performance." "97% of the time, premature optimization is the root of all evil." Yadda yadda. –  Apr 19 '13 at 01:54
  • If you keep the handler private, using anonymous delegates will make any call thread safe because worst case scenario it will call the empty delegates. Storing temp variable for the handler won't be necessary. – ForceMagic Dec 11 '13 at 21:45
0

I don't normally use C# but it seems like good practice to me. This is a textbook application of the 'Null Object' pattern. As noted, this will cost in terms of performance when the empty delegate actually runs, but gains in terms of performance every other time, when you don't have to check for an explicit null - so it should also be a net win performance-wise whenever the frequency of empty delegates is low.

Karl Knechtel
  • 62,466
  • 11
  • 102
  • 153