C# – Benefits of Using .NET Style Delegates Compared to Custom Ones

I know about C# events and delegates. I find them incredibly useful for event-driven sub-systems. One thing I don't understand, however, is why all the .NET documentation for events uses a very specific pattern for delegates:

public class Foo
{
    public class CustomEventArgs : System.EventArgs
    {
        ...
    }

    public delegate void CustomEventHandler(object sender, EventArgs e);
    public event CustomEventHandler CustomEvent;

    public void OnCustomEvent(int x, float y, bool z)
    {
        if (CustomEvent != null)
            CustomEvent(this, new CustomEventArgs(x, y, z));
    }
}

I find this method of declaring and using delegates incredibly unsecure and clunky compared to using custom delegates. For example:

public class Foo
{
    public delegate void CustomEventHandler(Foo foo, int x, float y, bool z);
    public event CustomEventHandler CustomEvent;

    public void OnCustomEvent(int x, int y, int z)
    {
        if (CustomEvent != null)
            CustomEvent(this, x, y, z);
    }
}

Besides the obvious advantage of "It's the pattern most familiar to .NET programmers", I fail to see any other practical advantages to using delegates with the void CustomEventHandler(object sender, CustomEventArgs e) signatures as opposed to custom delegates. Mainly, you have the following advantages with custom delegates:

You can guarantee the sender is of a specific type.
You don't need a whole new class just to pass the event data, leading to messier code and name pollution

I would be interested to know if there are any other advantages to using the .NET pattern for events and delegates that I might be missing.

Edit:

Mainly as a response to @MainMa, I wanted to give a more concrete example. The example is a simple representation of a character with a concept of health and death. The character throws events when its hurt, or dies. Consider the scenario below, using custom delegates:

public class Character
{
    public delegate void DeathEventHandler(Character character);
    public delegate void HurtEventHandler(Character character, float damage);

    public event DeathEventHandler DeathEvent;
    public event HurtEventHandler HurtEvent;

    public bool IsDead { ... }

    public void ApplyDamage(float damage)
    {
        ...
        OnHurt(damage);
        ...
        if (IsDead)
            OnDeath();
    }

    public void OnHurt(float damage)
    {
        if (HurtEvent != null)
            HurtEvent(this, damage);
    }

    public void OnDeath()
    {
        if (DeathEvent != null)
            DeathEvent(this);
    }
}

To me, this is simple. It separates concerns, and enforces encapsulation. A handler that is only concerned about the death of a character doesn't need to know how much damage was applied to the character before death. So on and so forth.

Now compare this to the .NET pattern:

public class Character
{
    public class CharacterHurtEventArgs : EventArgs
    {
        public float Damage { ... }
        public bool Dead { ... }
    }

    public event EventHandler<CharacterHurtEventArgs> HurtEvent;
    public event EventHandler<EventArgs> DeathEvent; // No args ...?

    // ... The rest is the same story as above more or less ...
}

Using this, I've just added an extra class. The handler still needs different delegates for these events, and there is no guarantee that the object which died was a character. Anything could throw this.

Of course, going by @MainMa's suggestion, we could change this into:

public class Character
{
    public class HealthConditionEventArgs : EventArgs
    {
        public float Damage { ... }
        public bool Dead { ... }
        public float Hitpoints { ... }
        ...
    }

    public event EventHandler<HealthConditionEventArgs> HealthConditionChangedEvent;

    public void ApplyDamage(float damage)
    {
        ...
        HealthConditionChangedEventArgs e = new HealthConditionChangedEventArgs();
        ...
        OnHealthConditionChanged(e);
        ...
    }

    public void OnHealthConditionChanged(HealthConditionEventArgs e)
    {
        if (HealthConditionChangedEvent != null)
            HealthConditionChangedEvent(e);
    }
}

This is slightly neater, but it eliminates separation of concerns. Now if the handler only cares about death, it will have to receive all hurt events. Of course, we could have separate events taking in the same event args, but you'd still be passing around the entire character health condition as part of the event. Again, if you care only about death, there is no need for the handler to know any more than the fact that the character died.

In my opinion, the first example, however, resolves all these encapsulation issues. Discrete delegates for discrete events, with only the necessary information passed in each event. No new classes either.

Best Answer

Let's imagine an example of a presentation layer. The layer has mouse interactions: every time the user moves a mouse, an event telling which mouse moved (imagine the user may have multiple mice) and how the mouse moved can be raised.

With your approach, the draft code would be:

public delegate void MouseMovedHandler(
    PresentationLayer l, Guid mouseId, int fromX, int fromY, int toX, int toY);

It will obviously be quickly refactored into:

public delegate void MouseMovedHandler(
    PresentationLayer l, Guid mouseId, Coord from, Coord to);

But then, you'll find that you want to know, for instance, the distance between from and to. So you'll create a Line class, defined by two points, and your delegate will become:

public delegate void MouseMovedHandler(PresentationLayer l, Guid mouseId, Line motion)

Better. Now, what if we want to be able to make a difference between a simple line and a motion of a specific, identified mouse? Right, we'll end up creating the class:

class MouseMotion : Line
{
    public Mouse Mouse { get; }
    public Line Motion { get; }
    ...
}

public delegate void MouseMovedHandler(PresentationLayer l, MouseMotion motion)

Now, there is a location in your app where you need to track every possible input: mouse motion, mouse button clicks, keyboard strokes. You need that because your application starts background processing when the user is not doing anything, but should stop it and free the resources to the UI if the user is using your app.

This means that now, you need something common between MouseMotion, MouseButtonChange and KeyboardStroke. Something like IEvent. Or wouldn't it be easier to simply use EventArgs?

So you end up with:

class MouseMotionEventArgs: EventArgs { ... }
public delegate void MouseMovedHandler(PresentationLayer l, MouseMotionEventArgs motion)

Except that you could avoid the delegate, as explained at the beginning of the answer. You can now either stick with the delegate in order to have your strongly typed PresentationLayer you probably don't need most of the time, or replace it with a lousy object by using EventHandler<MouseMotionEventArgs>.

The previous version of the answer, for the sake of completeness:

Is there a reason you create a CustomEventHandler instead of using EventHandler<CustomEventArgs>?

With the generic EventHandler<T>, the code starts looking like this:

public class Foo
{
    public event EventHandler<3DPointChangedEventArgs> CustomEvent;

    public void OnCustomEvent(3DPoint point)
    {
        if (CustomEvent != null)
        {
            CustomEvent(this, (3DPointChangedEventArgs)point);
        }
    }
}

Now, compare it to the code you suggested:

Yours is longer. Chances are, your colleagues will refactor x, y and z into 3DPoint object, making it a bit shorter.
Yours doesn't match the convention used in .NET Framework. Therefore, it will be difficult to know how to use it.
The one used in .NET Framework makes it possible “to write a single event handler for multiple events, regardless of sender or event.”

Talking specifically about name pollution, having *EventArgs classes is not that bad. Those names are usually very clear and express the intention of the author and cannot be confused with other business objects.

Answering your last edit, there are three points to consider:

It is not necessarily useful to separate the death from the health. The most basic representation of health could be a number; for instance, 0.0 can represent death and 100.0 can correspond to the full health.

Of course, a number would better be replaced by Health class which can then contain business rules which can be modified through time. For example, if it is a game, further features may include additional health slots, making it possible to have, say, 120% of health.

This may create a performance issue: one event less means that if something is listening to the death events, it will now be notified about all the health change events. I wouldn't be concerned about that early, and wait until it becomes an actual bottleneck. After all, what if the performance becomes even better, now that we don't have a condition in ApplyDamage()?
If the author decides to keep both events, death event can use EventArgs, making it possible to use EventArgs.Empty. This makes one delegate less to write.
Who listens to the event? Let's imagine the world listens to it in order to remove objects a few minutes after their death (which makes sense, since objects may not be aware of the world, and so won't be able to remove themselves from it).

This also means that we don't really care if the dead object is a Character or a Trebuchet or a WarShip or a Horse : Animal. This means that it is OK to lose types when transmitting the information through the event: the removal will be the same for a paladin or a sheep.

Here's the final result which applies the point 1 and 3:

public class Health
{
    private double value;

    public Health(double value) { ... }

    public void ApplyDamage() { ... }
}

public class Character
{
    public event EventHandler<HealthEventArgs> HealthChanged;

    public Health Health { get { return this.health; } }

    public void ApplyDamage(double damage)
    {
        ...
        this.health.ApplyDamage(damage);
        ...
    }

    public void OnHealthChange(float damage)
    {
        if (HurtEvent != null)
        {
            HurtEvent(this, damage);
        }
    }
}

The events handling can then be done like this, making abstraction of the type of the world object being removed:

public class World
{
    public void MarkAsDead(object worldObject)
    {
        // Wait for five minutes.
        this.elements.Remove(worldObject);
    }
}

NSubstitute

var mockedItem = Substitute.For<INeedToBeMocked>();
mockedItem.Received().EventRaised += Arg.Any<EventHandler>();

Rhino Mocks

var mockedItem = MockRepository.GenerateMock<INeedToBeMocked>();
mockedItem.AssertWasCalled(x => x.EventRaised += Arg.Is.Anything);

Best Answer

Related Solutions

C# – What are the disadvantages of using/writting a .Net Custom Membership Provider

C# – How to Test Eventhandler Subscription

NSubstitute

Rhino Mocks

Related Topic