# What do you think of my EventAggregator implementation in C#?

I was looking for a simple Event Aggregator to use in my app, but I found people get very complicated very quickly, so I thought this was easy enough to write myself. It seems to work, but I was wondering what you think and if you would change or add anything. (I know some of them raise events back on the same thread that it was registered, but for what I was planning I don't need this.)

public class EventAggregator
{
private Dictionary<Type, List<WeakReference>> _listeners;

private object _syncLock;

public EventAggregator()
{
_listeners = new Dictionary<Type, List<WeakReference>>();
_syncLock = new object();
}

public EventAggregator Register<TEvent>(Action<TEvent> eventAction)
{
LockAround(() =>
{
var listeners = GetListeners(typeof(TEvent));
listeners.RemoveAll(wr => !wr.IsAlive || (Action<TEvent>)wr.Target == eventAction);
});
return this;
}

public void Raise<TEvent>(TEvent ev)
{
List<WeakReference> targets = null;
LockAround(() =>
{
var listeners = GetListeners(typeof(TEvent));
targets = listeners
.Where(wr => wr.IsAlive)
.ToList();
});

targets.ForEach(wr => ((Action<TEvent>)wr.Target)(ev));
}

private void LockAround(Action action)
{
lock (_syncLock)
{
action();
}
}

private List<WeakReference> GetListeners(Type type)
{
List<WeakReference> result;
if (_listeners.TryGetValue(type, out result))
{
return result;
}
result = new List<WeakReference>();
_listeners[type] = result;
return result;
}
}


This is how I plan to use it:

class Program
{
static void Main(string[] args)
{
var ea = new EventAggregator();

var view = new View(ea);
var p = new Presenter(ea);

view.Name = "Name1";
view.Name = "Name2";

}
}

interface INameChangeEvent
{
string OldName { get; set; }

string NewName { get; set; }
}

public class NameChangeEvent : EventArgs, INameChangeEvent
{
public NameChangeEvent(string oldName, string newName)
{
this.OldName = oldName;
this.NewName = newName;
}

public string OldName { get; set; }

public string NewName { get; set; }
}

class View
{
private string _name;
private EventAggregator _ea;

public View(EventAggregator ea)
{
_ea = ea;
}

public string Name
{
get
{
return _name;
}
set
{
_ea.Raise<INameChangeEvent>(new NameChangeEvent(_name, value));
_name = value;
}
}
}

class Presenter
{
public Presenter(EventAggregator ea)
{
ea.Register<INameChangeEvent>((ev) =>
Console.WriteLine(string.Format("View name was changed from {0} to {1}",
ev.OldName, ev.NewName)));
}
}


## migrated from stackoverflow.comOct 17 '11 at 9:38

This question came from our site for professional and enthusiast programmers.

• I'll embarrassingly admit a bit of TL;DR for the moment, but at a quick glance, I'd make the two private variables readonly as well. – Jesse C. Slicer Oct 17 '11 at 15:12

I haven't thoroughly looked at it but these are my initial thoughts.

Minor points

I would initialise the private variables inline like this private object _syncLock = new Object(); because they don't depend on parameters given to the constructor.

For readability I wouldn't use the LockAround method, I would just lock _syncObject.

In GetListeners is there any reason you are adding a list to the dictionary when no events have been registered for that type? Also, you could make it generic by pulling in the typeof from Raise like so _listeners.TryGetValue(typeof(TEvent), out result).

In Register<TEvent> what is the purpose of (Action<TEvent>)wr.Target == eventAction? I think if someone wants to register the same event handler twice then they should be able to.

You might want to make your event aggregator a singleton or use a dependency injection framework to ensure there is only ever one instance in existence.

Worries

In Raise<TEvent> the following line worries me targets.ForEach(wr => ((Action<TEvent>)wr.Target)(ev));. I'm not sure there is any guarantee the weak reference will still be alive at this point. You probably want to solidify the reference and check that it is not null before calling it.

Finally

Have you looked at Microsoft's event aggregator and unity dependency injection framework? I think they probably do what you are looking for and will save you from reinventing the wheel.

• +1 the LockAround method does not have any advantage atm (in fact it requires 2 more characters) – MattDavey Oct 18 '11 at 8:45
• Thanks, The GetListeners should maybe be called GetOrCreateListeners this is only used in one place and the idea is to create the list if it doesnt exist – Andre Oct 18 '11 at 9:01
• I also agree with your observation, i think better would be to inside the foreach (assign the object) (so that its referenced) then check null and isalive? – Andre Oct 18 '11 at 9:02
• Finally my reasoning for re inventing the wheel in this case was exactly that is that using one that is frameworked may be really overcomplicating things?, i thought this was really easy to implement – Andre Oct 18 '11 at 9:03
• There is no need to use IsAlive if you assign the object and check it is not null. In fact checking IsAlive == true is not recommended, see: msdn.microsoft.com/en-us/library/… – Joey Oct 18 '11 at 9:05