is this a good way of using events?

Question

So I'm currently trying to learn more about events using C# and the example that I just write seems a bit.. It seems a bit too much? I feel like you could accomplish the same thing with way less code. I just have a hard time seeing why an event in this case would be useful? Maybe I just did it wrong.

Is this a good way of using event? Could it be improved?

class Program
{
    static void Main(string[] args)
    {
        Player player = new Player();
        player.OnLevelUp += Player_OnLevelUp;
        Console.WriteLine(player.Level);

        for (int i = 0; i < 10; i++)
        {
            player.SetXp(10);
        }

        Console.ReadLine();
    }

    private static void Player_OnLevelUp(object sender, EventArgs e)
    {
        var player = (Player)sender;
        player.Level += 1;

        Console.WriteLine("You leveled up!");
        Console.WriteLine(player.Level);
    }
}

public class Player
{
    public event EventHandler OnLevelUp;
    public string Name { get; set; }
    public int Level { get; set; }
    public int XP { get; set; }
    
    private int _nextLevelXp = 50;

    private void LevelUp()
    {
        OnLevelUp?.Invoke(this, EventArgs.Empty);
    }

    public void SetXp(int xp)
    {
        XP += xp;
        if (XP >= _nextLevelXp)
        {
            LevelUp();
            _nextLevelXp *= 2;
        }
    }
}

Answer 1

What you've done is conceptually wrong

Events are there to allow other objects to be notified and react when something happens.

That said, your solution works and does what you want, at least at a first short glance, but it's misusing the event system for something it should not be doing.

Events are there to allow objects with different responsibilities to react to the change of state of the originator.

What you've done is moved the responsibility of the player class (the actual level-up - the incrementing of the level) to another class (Program). The Program has different responsibility than maintaining the inner state of the player.

The player is a class that has its own state and methods that change it (the whole SetXP and LevelUp methods). The Program is just using those methods (and sometimes reading that state) to make the player do something and uses the event to manage its own responsibility, which is to write about the level up when it happens.

The code just needs a small change and do something like:

    private void LevelUp()
    {
        Level++; //this leaves the responsibility of leveling up to the Player
        OnLevelUp?.Invoke(this, EventArgs.Empty);
    }

and then leave the OnLevelUp as:

    private static void Player_OnLevelUp(object sender, EventArgs e)
    {
        //this allows the Program to only be responsible for it's own state
        var player = (Player)sender;
        Console.WriteLine("You leveled up!");
        Console.WriteLine(player.Level);
    }

To be well enough formed.

Handle the LevelUp in the LevelUp method and then let the Program know through the OnLevelUp event and let the Program fulfil its responsibility by writing about it to the console.

---

The SOLID principles

Now, since you're learning, let's make a bit more sense of what I'm saying.

We programmers have some paradigms that we try to follow to make each other's lives easier. They are not unwritten - in fact, there are so many books about them it would fill a really large library.

One of those paradigms that people, who work with object-oriented languages, will (or should) start learning as soon as reasonable are the SOLID principles.

Those principles have really far-reaching consequences. We shouldn't be treating them like gospel and be holier than thou, but we should be aware of them.

One of the SOLID principles is The Principle of Single Responsibility - a class/module/function should have one responsibility.

It is a general rule and we should not take it too far, but it is a good, if abstract, rule.

Under this principle - we call it SRP for short - the class Player is responsible for representing the player and his state and The Program is responsible for letting the user play the game (which includes using the methods exposed by the player class and writing about what's happening to the Console window).

What you've done is violated this principle by letting the Program mess with the player directly.

The thing this practically achieves is Separation of Concerns. We separate the logic and state to the Player class that is responsible for it, and the logic of the game to another class that is responsible for its own thing and uses the separated logic through the interface (methods and properties and fields) it exposes.

It is not such a big deal here, in a tutorial program. But, once you start working with large objects that have lots of properties and fields and need to maintain a complex state, it can be problematic.

Especially when you don't have control over who will be using the class. Letting some unknown party do something to the state of your object without your control can be a disaster.

Imagine you write a class that works as an HTTP client. It is complex and has a lot of events and properties and fields. But, you're not the one who will be using it. There will be some other programmer in this large team you're working in (or just someone using your library), who will be using it to do something - probably send an HTTP request and receive a response...

Is it really a good idea to let him change the internal state of your client willy nilly? Or to make the other programmer responsible for changing the values your class is responsible for (your client's own internal status)?

That's why the client has an event. Let the other programmer subscribe his methods to your event and let his classes react to the changes in the state of the client, but change the state with the methods you've written.

Also - there can be multiple consumers subscribed to the event.

If I start building on your example, there can be an object that draws the player on the screen (Graphics), and when the OnLevelUp is fired it, starts a level-up animation.

Then there can be objects representing the Fight the player is in, that are subscribed to the OnLevelUp event. When he levels up, the hypothetical Fight class, responsible for the ongoing fight, might check if his opponents are too weak and might trigger them to try to flee from the player...

In your case, it's the Program subscribing to the Player's OnLevelUp event and writing about the change to the Console window. It is the responsibility of the Program to write everything on the screen. It is the responsibility of the player object to maintain its own state and represent a player.

Of course, this is just an example. There are probably reasons why you might want to change the state of another object in an event. The pundits have probably already sharpened their pens, and gods willing, will inform us of them in their own answers. Your usage, however, is clearly not it.

---

WHY SO MUCH CODE

Well, mainly because the system needs to be universal enough for a lot of people to use.

The whole program can be - of course - written with much less code, but it is just a tutorial example to let you know about how events work.

It is like taking two perfectly good pieces of two by four and hammering a nail through them to show us how nailing works. In reality, you could also just glue them together or use a piece of string to tie them together, but the author wanted to show you nails and how they work...

---

POTENTIAL PROBLEMS

If you change the inner state of one object in another object in an uncontrolled manner, the problems will start to crop up as your program grows.

Imagine you persist in that practice and have multiple classes that do this. Even relatively small programs devolve into a nightmare because you have to keep a tally of which class changes what in another class. Now imagine you have a hundred classes that do that...

Answer 2

Events should report when something changed.

edit as pointed out events can be before or after. I'm just used to mainly dealing with after but events can happen before and some events that happen before even allow for the process to be cancelled. But the main point is events are fired for something happening and events don't make the change happen.

In your example of levelup where the event is the one that actually increases the level property is extremely rare. I would go so far as saying incorrect.

What typically happens is the Level property is increased then an event is fired to tell everyone that is listening to the level property changed. Maybe an item is available when a player is at a specific level and it only shows or is enabled at a specific level. That code could subscribe to the event and listen for when the player cross a specific level.

Also the event args should be about the event that happen. Instead of EventArgs.Empty you could send out a custom event that would contain the event data that is important. In this case the player Name and level that was changed to.

Events really shine to decouple when something happens to the consumer and when there are multiple consumers.