Raising C# events with an extension method - is it bad?

We're all familiar with the horror that is C# event declaration. To ensure thread-safety, the standard is to write something like this:

public event EventHandler SomethingHappened;
protected virtual void OnSomethingHappened(EventArgs e)
{            
    var handler = SomethingHappened;
    if (handler != null)
        handler(this, e);
}

Recently in some other question on this board (which I can't find now), someone pointed out that extension methods could be used nicely in this scenario. Here's one way to do it:

static public class EventExtensions
{
    static public void RaiseEvent(this EventHandler @event, object sender, EventArgs e)
    {
        var handler = @event;
        if (handler != null)
            handler(sender, e);
    }
    static public void RaiseEvent<T>(this EventHandler<T> @event, object sender, T e)
        where T : EventArgs
    {
        var handler = @event;
        if (handler != null)
            handler(sender, e);
    }
}

With these extension methods in place, all you need to declare and raise an event is something like this:

public event EventHandler SomethingHappened;

void SomeMethod()
{
    this.SomethingHappened.RaiseEvent(this, EventArgs.Empty);
}

My question: Is this a good idea? Are we missing anything by not having the standard On method? (One thing I notice is that it doesn't work with events that have explicit add/remove code.)

It will still work with events that have an explicit add/remove - you just need to use the delegate variable (or however you've stored the delegate) instead of the event name.

However, there's an easier way to make it thread-safe - initialize it with a no-op handler:

public event EventHandler SomethingHappened = delegate {};

The performance hit of calling an extra delegate will be negligible, and it sure makes the code easier.

By the way, in your extension method you don't need an extra local variable - you could just do:

static public void RaiseEvent(this EventHandler @event, object sender, EventArgs e)
{
    if (@event != null)
        @event(sender, e);
}

static public void RaiseEvent<T>(this EventHandler<T> @event, object sender, T e)
    where T : EventArgs
{
    if (@event != null)
        @event(sender, e);
}

Personally I wouldn't use a keyword as a parameter name, but it doesn't really change the calling side at all, so do what you want :)

EDIT: As for the "OnXXX" method: are you planning on your classes being derived from? In my view, most classes should be sealed. If you do, do you want those derived classes to be able to raise the event? If the answer to either of these questions is "no" then don't bother. If the answer to both is "yes" then do :)

Now C# 6 is here, there is a more compact, thread-safe way to fire an event:

SomethingHappened?.Invoke(this, e);

Invoke() is only called if delegates are registered for the event (i.e. it's not null), thanks to the null-conditional operator, "?".

The threading problem the "handler" code in the question sets out to solve is sidestepped here because, like in that code, SomethingHappened is only accessed once, so there is no possibility of it being set to null between test and invocation.

This answer is perhaps tangential to the original question, but very relevent for those looking for a simpler method to raise events.

[Here's a thought]

Just write the code once in the recommended way and be done with it. Then you won't confuse your colleagues looking over the code thinking you did something wrong?

[I read more posts trying to find ways around writing an event handler than I ever spend writing an event handler.]

Less code, more readable. Me like.

If you're not interested in performance you can declare your event like this to avoid the null check:

public event EventHandler SomethingHappened = delegate{};

You're not "ensuring" thread safety by assigning the handler to a local variable. Your method could still be interrupted after the assignment. If for example the class that used to listen for the event gets disposed during the interruption, you're calling a method in a disposed class.

You're saving yourself from a null reference exception, but there are easier ways to do that, as Jon Skeet and cristianlibardo pointed out in their answers.

Another thing is that for non-sealed classes, the OnFoo method should be virtual which I don't think is possible with extension methods.

To take the above answers a step further you could protect yourself against one of your handlers throwing an exception. If this were to happen then the subsequent handlers wouldn't be called.

Likewise, you could taskify the handlers to prevent a long-running handler from causing an excessive delay for the latter handlers to be informed. This can also protect the source thread from being hijacked by a long-running handler.

  public static class EventHandlerExtensions
  {
    private static readonly log4net.ILog _log = log4net.LogManager.GetLogger(MethodBase.GetCurrentMethod().DeclaringType);

    public static void Taskify(this EventHandler theEvent, object sender, EventArgs args)
    {
      Invoke(theEvent, sender, args, true);
    }

    public static void Taskify<T>(this EventHandler<T> theEvent, object sender, T args)
    {
      Invoke(theEvent, sender, args, true);
    }

    public static void InvokeSafely(this EventHandler theEvent, object sender, EventArgs args)
    {
      Invoke(theEvent, sender, args, false);
    }

    public static void InvokeSafely<T>(this EventHandler<T> theEvent, object sender, T args)
    {
      Invoke(theEvent, sender, args, false);
    }

    private static void Invoke(this EventHandler theEvent, object sender, EventArgs args, bool taskify)
    {
      if (theEvent == null)
        return;

      foreach (EventHandler handler in theEvent.GetInvocationList())
      {
        var action = new Action(() =>
        {
          try
          {
            handler(sender, args);
          }
          catch (Exception ex)
          {
            _log.Error(ex);
          }
        });

        if (taskify)
          Task.Run(action);
        else
          action();
      }
    }

    private static void Invoke<T>(this EventHandler<T> theEvent, object sender, T args, bool taskify)
    {
      if (theEvent == null)
        return;

      foreach (EventHandler<T> handler in theEvent.GetInvocationList())
      {
        var action = new Action(() =>
        {
          try
          {
            handler(sender, args);
          }
          catch (Exception ex)
          {
            _log.Error(ex);
          }
        });

        if (taskify)
          Task.Run(action);
        else
          action();
      }
    }
  }