8
\$\begingroup\$

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?

\$\endgroup\$

3 Answers 3

5
\$\begingroup\$

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.
\$\endgroup\$
5
  • \$\begingroup\$ Great feedback and nicely spotted on the bug. Thanks mate. One question though. What's the advantage of changing the private member to an IList<T> vs List<T>? \$\endgroup\$ Commented Nov 23, 2015 at 10:49
  • \$\begingroup\$ I agree with most of your points, but what's the reason you don't like the backing implementation for an IList<T> being List<T>? \$\endgroup\$
    – hangy
    Commented Nov 23, 2015 at 10:52
  • \$\begingroup\$ Because you should always code against interfaces rather then implementations. If you implement IList<T> it makes sense to restrict the underlaying list to the same interface. You don't use/need any of the List<T> methods. \$\endgroup\$
    – Heslacher
    Commented Nov 23, 2015 at 10:55
  • 2
    \$\begingroup\$ "The RemoveAt() could be simplified by calling the Remove() method" Don't do that! The Remove() method needs to locate the item before removing it which is a O(n) operation. RemoveAt() already has the index so it should be a O(1) operation. Instead, call RemoveAt() from Remove(). \$\endgroup\$ Commented Nov 23, 2015 at 12:07
  • \$\begingroup\$ I think I'm okay with the way Clear is already implemented. The whole point of the class is that I don't have to listen to different kinds of events (I don't care how the items are added / removed, just that they are). Also, to put the Clear before the loop I'd have to make a copy of the list. \$\endgroup\$ Commented Nov 23, 2015 at 13:34
5
\$\begingroup\$

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;
}
\$\endgroup\$
1
  • 1
    \$\begingroup\$ I implemented this one exactly as described after reading your previous comment. Thanks :) \$\endgroup\$ Commented Nov 23, 2015 at 13:30
1
\$\begingroup\$
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));
}
\$\endgroup\$
1
  • \$\begingroup\$ Is there a sensible reason you'd ever want to do what the OP did there? I'm no expert and genuinely curious. \$\endgroup\$
    – Clonkex
    Commented Sep 9, 2021 at 23:34

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.