More struggles with C# Events

Question

I'm working on a simple little program/game where I'm simulating a standoff. I want an event to trigger when one of the enemy objects aims at the other (so that it can adjust its trust properties accordingly).

In the 'Enemy' class, I've put the following (I've cut out the parts with properties and constructors so this doesn't take up so much space):

    public event EventHandler RexAim;
    public void OnRexAim()
    {
        if (RexAim != null)
            RexAim(this, EventArgs.Empty);
    }

    public void RexAimAt()
    {
        Console.WriteLine("This is the aiming method leaping into action.");
        OnRexAim();
    }

And then in the Program class, I have this:

class Program
{
    static void Main(string[] args)
    {
        // Constructing the characters so I can use them in things:
        Enemy rex = new Enemy(100, -100, 5, 2, "Rex");
        Enemy pearl = new Enemy(100, -50, -5, 0, "Pearl");
        Enemy archie = new Enemy(75, -100, 0, 3, "Archie");
        Enemy betty = new Enemy(100, -75, 0, 5, "Betty");

        pearl.RexAim += HandleRexAim;

        rex.RexAimAt();

    }

    public static void HandleRexAim(object sender, EventArgs eventArgs)
    {
        Console.WriteLine("This should show up if the event worked properly.");
    }

}

But when I run it, all I get is the "This is the aiming method leaping into action" displayed.

I'm sure I'm making a very elementary mistake here (I'm a beginner at this), but I just can't wrap my head around what's not working.

Thanks in advance for your patience with someone so new to this!

You have it backwards. If `rex` is triggering the event then you need to subscribe to `rex.RexAim`. — Matt Burland, Apr 20 '18 at 16:27
It's off-topic, but your `OnRexAim` is not thread-safe, it should be `OnRexAim() => this.RexAim?.Invoke( this, EventArgs.Empty );` instead. — Dai, Apr 20 '18 at 16:29
@Dai: that's not any more threadsafe. It's just syntactic sugar for what the OP already has. In any case, this program doesn't have threads, so it doesn't matter. — siride, Apr 20 '18 at 16:36
@siride It is thread-safe: the `?.` operator creates a local copy, which prevents race-conditions if the event's backing-field is overwritten. See here: https://stackoverflow.com/questions/35784874/is-c-sharp-6-elvis-op-thread-safe-if-so-how/35785415 — Dai, Apr 20 '18 at 16:42
Oh my goodness. Thanks so much! It was as I feared; a very simple mistake I was making. — Kurtillius, Apr 20 '18 at 16:44
@Dai: you are correct that it does introduce a temporary, but that doesn't really make it threadsafe. Read the commentary on the top answer on the question you linked. There are still a large number of thread-related issues. — siride, Apr 20 '18 at 16:53
@siride Indeed, I should have said "null-safe in a multithreaded scenario" instead of the broader, and context-sensitive term "thread-safe" — Dai, Apr 20 '18 at 17:11
@Kurtillius Following on @MattBurland, `res.RexAimAt` calls `rex.OnRexAim` which tests `rex.RexAim` which is `null` _because you didn't assign anything to_ `rex.RexAim`. — NetMage, Apr 20 '18 at 17:18
Aside from the problem with handling events, it's really not clear to me what you are trying to achieve and whether or not events are the right way to handle this. I suspect not. It would seem to make more sense to be if `RexAimAt` (or maybe that should just be `AimAt`) took an argument that was the person being aimed at, if that is indeed the purpose of that function. — Matt Burland, Apr 20 '18 at 17:18

score 2 · Accepted Answer · answered Apr 20 '18 at 17:28

It's a little unclear what you are actually trying to achieve here, but I suspect that events aren't necessarily the right tool here. If I understand it you need an Enemy to be able to AimAt another Enemy and that will update some state (you call it Trust) in the Enemy being aimed at. It seems to me that a better approach would be to do something like:

public class Enemy
{
    public void AimAt(Enemy opponent)
    {
        Console.WriteLine($"{Name} is aiming at {opponent.Name}");
        opponent.AimedAt(this);
    }

    private void AimedAtBy(Enemy opponent)
    {
        // update trust between this and opponent
        Console.WriteLine($"{Name} trusts {opponent.Name} less because they are pointing a gun at them!");
    }
}

Then you'd do something like:

Enemy rex = new Enemy(100, -100, 5, 2, "Rex");
Enemy pearl = new Enemy(100, -50, -5, 0, "Pearl");
Enemy archie = new Enemy(75, -100, 0, 3, "Archie");
Enemy betty = new Enemy(100, -75, 0, 5, "Betty");

and then:

rex.AimAt(pearl);

Should output:

Rex is aiming at Pearl
Pearl trusts Rex less because they are pointing a gun at them!

There are a lot more variables that might go into a design like this, but it's a starting point.

A really good resource to read through is this series by Eric Lippert:

https://ericlippert.com/2015/04/27/wizards-and-warriors-part-one/

That's a really good point! Thanks so much for explaining that. I will definitely check Lippert out! — Kurtillius, Apr 21 '18 at 14:58

score 0 · Answer 2 · answered Apr 20 '18 at 18:02

    pearl.RexAim += HandleRexAim;

    rex.RexAimAt();

This is your problem. You are attaching to the pearl.RexAim event. Then you call rex.RexAimAt, which fires rex.RexAim event, which has nothing attached to it.

To see the output you expect, change it to "pearl.RexAimAt();".

Please read @Matt Burland's answer though for more in depth information on events/suggestions.

More struggles with C# Events

2 Answers2