C# Delegate events bad practice?

Question

I use a lot of custom events in my code, and have been declaring them like this

public delegate void ImageDownloadingEvent(Waypoint waypoint);
public event ImageDownloadingEvent ImageDownloading;

then firing them like this

if (ImageDownloading != null)
    ImageDownloading(waypoint);

What I'd like to know is, is this bad practice? Or a bad way of doing things? If so, why? And what would be a better approach?

Thanks for any help, just trying to improve my coding skills

vcsjones · Accepted Answer · 2016-09-06T13:32:34.753

Well it's up to you to decide if events are the right pattern to use for a given scenario. Like anything, they can be used correctly or they can become a code smell. You can use a delegate of your own declaration, or one of the more generic ones like Func, EventHandler, or Action if you don't want to declare a new type for every event.

Your use of events is mostly correct. You want to copy the handler to a local. Eric Lippert has an explanation why on his blog.

So it becomes this:

var imageDownloading = ImageDownloading;
if (imageDownloading != null)
    imageDownloading(waypoint);

The C# 6 compiler can do this for you like so:

ImageDownloading?.Invoke(waypoint);

In this case the compiler knows that it should make a local copy, first.

That code is only necessary if the event is going to be accessed from multiple threads at the same time, which is pretty rarely a requirement. — Servy, Sep 06 '16 at 13:32

score 0 · Answer 2 · answered Sep 06 '16 at 13:32

You can do it like that, although in a multi-threaded environment the way you are raising them has the potential for a race condition. Therefore, the recommended way to raise an event is actually

var handler = ImageDownloading;
if (handler != null) handler(waypoint);

With C# 6 that can be a bit more concise using the null-conditional operator:

ImageDownloading?.Invoke(waypoint);

Again, only relevant when multi-threading is a concern. Most code isn't built for that case anyway.

Then there is the question whether you want to use custom delegates for every event, or custom EventArgs for every event (and declare them as EventHandler<T>). That's entirely up to you, but it is sort of a convention in .NET.

C# Delegate events bad practice?

2 Answers2