EventList<T> a generic list implementation that raises events when items are added and removed

Question

I created this class for a project when I wanted to have List<T> properties on a class and also listen for items being added and removed.

I looked at using BindingList<T> or ObservableCollection<T> but I felt that their collection changed events are too complicated. I simply wanted to know when items are added and removed.

public class EventList<T> : IList<T>
{
    private readonly List<T> _list;

    public EventList()
    {
        _list = new List<T>();
    }

    public EventList(IEnumerable<T> collection)
    {
        _list = new List<T>(collection);
    }

    public EventList(int capacity)
    {
        _list = new List<T>(capacity);
    }

    public event EventHandler<EventListArgs<T>> ItemAdded;
    public event EventHandler<EventListArgs<T>> ItemRemoved;

    private void RaiseEvent(EventHandler<EventListArgs<T>> eventHandler, T item, int index)
    {
        var eh = eventHandler;
        eh?.Invoke(this, new EventListArgs<T>(item, index));
    }

    public IEnumerator<T> GetEnumerator()
    {
        return _list.GetEnumerator();
    }

    IEnumerator IEnumerable.GetEnumerator()
    {
        return GetEnumerator();
    }

    public void Add(T item)
    {
        var index = _list.Count;
        _list.Add(item);
        RaiseEvent(ItemAdded, item, index);
    }

    public void Clear()
    {
        for (var index = 0; index < _list.Count; index++)
        {
            var item = _list[index];
            RaiseEvent(ItemRemoved, item, index);
        }

        _list.Clear();
    }

    public bool Contains(T item)
    {
        return _list.Contains(item);
    }

    public void CopyTo(T[] array, int arrayIndex)
    {
        _list.CopyTo(array, arrayIndex);
    }

    public bool Remove(T item)
    {
        var index = _list.IndexOf(item);

        if (_list.Remove(item))
        {
            RaiseEvent(ItemRemoved, item, index);
            return true;
        }

        return false;
    }

    public int Count => _list.Count;
    public bool IsReadOnly => false;

    public int IndexOf(T item)
    {
        return _list.IndexOf(item);
    }

    public void Insert(int index, T item)
    {
        _list.Insert(index, item);
        RaiseEvent(ItemRemoved, item, index);
    }

    public void RemoveAt(int index)
    {
        var item = _list[index];
        _list.RemoveAt(index);
        RaiseEvent(ItemRemoved, item, index);
    }

    public T this[int index]
    {
        get { return _list[index]; }
        set { _list[index] = value; }
    }
}

public class EventListArgs<T> : EventArgs
{
    public EventListArgs(T item, int index)
    {
        Item = item;
        Index = index;
    }

    public T Item { get; }
    public int Index { get; }
}

The typical use case would be something like this:

public class MyClass
{
    public MyClass()
    {
        MyItems = new EventList<string>();

        MyItems.ItemAdded += (sender, args) =>
        {
            // do something when items are added
        };

        MyItems.ItemRemoved += (sender, args) =>
        {
            // do something when items are removed
        };
    }

    public EventList<string> MyItems { get; } 
}

I'm posting this here for code review for general feedback, but also to ask if I've missed something obvious, like an alternative class in the .NET framework or a 3rd party library that might do the same sort of thing?

Answer 1

Bug

public void Insert(int index, T item)
{
    _list.Insert(index, item);
    RaiseEvent(ItemRemoved, item, index);
}  

I don't think that you want to raise an ItemRemoved event in the case of inserting an item.

---

In general I like your implementation. It is leightweight and is doing exactly what it should.

But for sure I have something to critizise:

• The name EventList<T> which reads like a list of events instead of a list with events. Because there is already a ObservableCollection<T> maybe an ObservableList<T> would be a better name.

• You implement IList<T> but have the underlaying list as a List<T> instead of an IList<T> like private readonly IList<T> _list;.

• The RemoveAt() could be simplified by calling the Remove() method like so

  public void RemoveAt(int index)
  {
      Remove(_list[index]);
  }  
  

  See @RomanReiner's answer here

• IMO the raising of multiple ItemRemoved events inside the Clear() method isn't that good because you are raising the event before the item is removed. You should either have an additional event ItemsCleared or you should raise the ItemRemoved events after the clearing of that list.

Answer 2

Your implementation of the Remove() traverses the list twice. First when you do

    var index = _list.IndexOf(item);

and then again when you call

    if (_list.Remove(item))

Both are O(n) operations.

Instead, once you obtained the index using the first statement call RemoveAt(index). This also allows you to remove the duplicate event raising code:

public bool Remove(T item)
{
    var index = _list.IndexOf(item);

    if (index < 0)
        return false;

    RemoveAt(index);
    return true;
}

Answer 3

private void RaiseEvent(EventHandler<EventListArgs<T>> eventHandler, T item, int index)
{
    var eh = eventHandler;
    eh?.Invoke(this, new EventListArgs<T>(item, index));
}

The conditial invocation will be compiled to something like

if (eh != null)
{
    eh.Invoke(this, new EventListArgs<T>(item, index));
}

So there is no reason/advantage in having another copy of the eventHandler variable in this case. The only reason for using a local copy of the event handler is to avoid the original event handler being set to null before the invocation, but the method's parameter won't change. Just use

private void RaiseEvent(EventHandler<EventListArgs<T>> eventHandler, T item, int index)
{
    eventHandler?.Invoke(this, new EventListArgs<T>(item, index));
}