23

I have three objects ObjectA has an ObjectB, ObjectB has an ObjectC. When ObjectC fires an event I need ObjectA to know about it, so this is what I've done...

public delegate void EventFiredEventHandler();

public class ObjectA
{
    ObjectB objB;

    public ObjectA()
    {
        objB = new ObjectB();
        objB.EventFired += new EventFiredEventHandler(objB_EventFired);
    }

    private void objB_EventFired()
    {
        //Handle the event.
    }
}

public class ObjectB
{
    ObjectC objC;

    public ObjectB()
    {
        objC = new ObjectC();
        objC.EventFired += new EventFiredEventHandler(objC_EventFired);
        objC.FireEvent();
    }

    public event EventFiredEventHandler EventFired;
    protected void OnEventFired()
    {
        if(EventFired != null)
        {
            EventFired();
        }
    }

    private void objC_EventFired()
    {
            //objC fired an event, bubble it up.
        OnEventFired();
    }
}

public class ObjectC
{
    public ObjectC(){}

    public void FireEvent()
    {
        OnEventFired();
    }

    public event EventFiredEventHandler EventFired;
    protected void OnEventFired()
    {
        if(EventFired != null)
        {
            EventFired();
        }
    }
}

Is this the proper way to handle this, or is there a better way? I don't want ObjectA to know about ObjectC at all, only that it raised an event.

Singleton
  • 3,701
  • 3
  • 24
  • 37
Tester101
  • 8,042
  • 13
  • 55
  • 78

3 Answers3

23

Another approach, is to wrap it using add/remove:

public class ObjectB
{
    ObjectC objC;

    public ObjectB()
    {
        objC = new ObjectC();
    }

    public event EventFiredEventHandler EventFired
    {
        add { this.objC.EventFired += value; }
        remove { this.objC.EventFired -= value; }
    }
}
BFree
  • 102,548
  • 21
  • 159
  • 201
4

That's the way I do it. however I would recommend change your firing mechanism to this to make it thread safe

protected void OnEventFired()
{
    var tmpEvent = EventFired;
    if(tmpEvent != null)
    {
        tmpEvent();
    }
}

This keeps it from failing if EventFired becomes null between the null check and the firing.

Also it is somewhat of a standard to follow the EventHandler pattern for your event delegates.

protected virtual void OnEventFired(EventArgs e)
{
    var tmpEvent = EventFired;
    if(tmpEvent != null)
    {
        tmpEvent(this, EventArgs.e);
    }
}

I was wrong about the threadsafe pattern, here is the full threadsafe event pattern

/// <summary>
/// Delegate backing the SomeEvent event.
/// </summary>
SomeEventHandler someEvent;

/// <summary>
/// Lock for SomeEvent delegate access.
/// </summary>
readonly object someEventLock = new object();

/// <summary>
/// Description for the event
/// </summary>
public event SomeEventHandler SomeEvent
{
    add
    {
        lock (someEventLock)
        {
            someEvent += value;
        }
    }
    remove
    {
        lock (someEventLock)
        {
            someEvent -= value;
        }
    }
}

/// <summary>
/// Raises the SomeEvent event
/// </summary>
protected virtual OnSomeEvent(EventArgs e)
{
    SomeEventHandler handler;
    lock (someEventLock)
    {
        handler = someEvent;
    }
    if (handler != null)
    {
        handler (this, e);
    }
}
Scott Chamberlain
  • 124,994
  • 33
  • 282
  • 431
  • @Mike Cheel As I said in the post, if the event is unsubscribed between the null check and the actual firing a Exception (NullRefrence I think) will be thrown. By assigning the event to a separate variable the event will still fire that last time if it was unsubscribed in the middle of this function. – Scott Chamberlain Dec 03 '10 at 19:48
  • Isn't the usual signature `protected virtual void OnMyEvent(MyEventArgs e)`? – Greg Dec 03 '10 at 19:50
  • @Scott Chamberlain: Wouldn't you get the NullReference exception either way? – Tester101 Dec 03 '10 at 19:51
  • @Scott Chamberlain: Didn't follow the EventHandler pattern, because I'm lazy and didn't want to type it. – Tester101 Dec 03 '10 at 19:53
  • @Tester101 That is what I was thinking. If anything if there are so many subs and unsubs to an event I would personally wrap it in a try and handle the exception. – Mike Cheel Dec 03 '10 at 19:55
  • @Scott Chamberlain Thanks for taking the time to post the correction. – Mike Cheel Dec 03 '10 at 20:00
  • There's no need to lock around any of the event assignments. Multicast delegates are immutable, and when you += a new instance is assigned. – BFree Dec 03 '10 at 20:01
  • @Scott Chamberlain: The thread safe stuff seems like a lot of extra work, couldn't I just wrap it in a try/catch and ignore any exceptions? I mean if nobodies listening, do I really care if the event fires or not? – Tester101 Dec 03 '10 at 20:03
  • Reference assignment is atomic, isn't it? Why place a lock around it? – Greg Dec 03 '10 at 20:08
  • @Greg I am not 100% sure but I assume it is because the += is not atomic on the add and remove so you do not want to be doing your assignment in the middle of that modification, but that is just a assumption on my part. – Scott Chamberlain Dec 03 '10 at 20:53
  • @Tester101, I guess you could, unless somehow calling the delegate while some other thread is in the middle of a += or -= operation causes undefined behavior. It may not throw a exception but it will not do what you want (take this with a grain of salt, this is 100% speculation and I do not know if this is actually what happens) – Scott Chamberlain Dec 03 '10 at 20:56
  • @BFree that is what I originally through too (see my striked out code) but I can not find any examples on the net when dealing with thread safety that does not include the lock on the += and -= – Scott Chamberlain Dec 03 '10 at 20:57
  • Your second example is almost correct, should be `tmpEvent(this, e);` instead of `tmpEvent(this, EventArgs.Empty);` – Greg Dec 08 '10 at 15:40
  • I'm not an expert on the matter, but I think that your third "threadsafe" example is completely unnecessary. The auto-implementation of `public event SomeEventHandler SomeEvent` is thread-safe (http://pastebin.com/ASXFMnZ6) and there is no need for the lock in `OnSomeEvent` because reference assignment is atomic. – Greg Dec 08 '10 at 15:50
1

As other answers have stated, this is they way to do it.

But you can go beyond!!! I've just implemented a good data structure on it, and it's like to give you a spin on it.

Would be nice to have an automatic event bubbling? You could implement it using Reflection. My way is to define an Interface/Base class which declares an event (or a set of events). Then, the parameterless constructor of a base class will iterate other its properties/fields, and register automatically the members events for event propagation.

There are some restriction on design, but if you have a deep structure and/or many (structured) events, it could be nice to have everything setup without any additional line of code.

An initial base class could be:

class BaseObject {
    public BaseObject() {
        FieldInfo[] fInfos = this.GetType().GetFields(...);

        foreach (FieldInfo fInfo in fInfos) {
            object fInfoValue = fInfo.GetValue(this, null);
            if (fInfoValue is BaseObject) {
                BaseObject bMemberObject = (BaseObject)fInfoValue;

                bMemberObject.MyEvent += new EventHandler(delegate() {
                    if (this.MyEvent != null)
                        MyEvent();
                });
            }
    }

    public event MyEvent = null;

}

Of course, as already suggested, follow the event delegate delegate(object sender, EventArgs args) (I've used a simpler event for clarity). Naturally, is implicit that you classes A, B and C derives directly from BaseObject.

Note that any logic could be implemented to bind structured events (you could be the nested event registration using the name and/or other reflected properties.

Luca
  • 11,646
  • 11
  • 70
  • 125