Implementing IList<T>, IList & INotifyCollectionChanged

Question

I posted this question over at Stackoverflow, which was off-topic.

I was using ObservableCollection in a number of places in my WPF application for very large arrays. This caused a problem as ObservableCollection did not take an argument for capacity like List did. This was an important optimization for locking down my memory usage and ensuring that these collections did not double to a much larger size than required.

So I implemented the following class:

public class ObservableList<T> : IList<T>, IList, INotifyCollectionChanged
{
    public event NotifyCollectionChangedEventHandler CollectionChanged;

    #region Properties
    private List<T> _List
    {
        get;
        set;
    }

    public T this[int index]
    {
        get
        {
            return _List[index];
        }
        set
        {
            if (index < 0 || index >= Count)
                throw new IndexOutOfRangeException("The specified index is out of range.");
            var oldItem = _List[index];
            _List[index] = value;
            OnCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Replace, value, oldItem, index));
        }
    }

    public int Count
    {
        get
        {
            return _List.Count;
        }
    }

    public bool IsReadOnly
    {
        get
        {
            return ((IList<T>)_List).IsReadOnly;
        }
    }

    bool IList.IsFixedSize
    {
        get
        {
            return ((IList)_List).IsFixedSize;
        }
    }

    object IList.this[int index]
    {
        get
        {
            return ((IList)_List)[index];
        }
        set
        {
            if (index < 0 || index >= Count)
                throw new IndexOutOfRangeException("The specified index is out of range.");
            var oldItem = ((IList)_List)[index];
            ((IList)_List)[index] = value;
            OnCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Replace, value, oldItem, index));
        }
    }

    public bool IsSynchronized
    {
        get
        {
            return ((IList)_List).IsSynchronized;
        }
    }

    public object SyncRoot
    {
        get
        {
            return ((IList)_List).SyncRoot;
        }
    }
    #endregion

    #region Methods
    private void OnCollectionChanged(NotifyCollectionChangedEventArgs args)
    {
        if (CollectionChanged != null)
            CollectionChanged(this, args);
    }

    public void AddRange(IEnumerable<T> collection)
    {
        _List.AddRange(collection);
        var iList = collection as IList;
        OnCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Reset));
    }

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

    public void Insert(int index, T item)
    {
        _List.Insert(index, item);
        OnCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Add, item, index));
    }

    public void RemoveAt(int index)
    {
        if (index < 0 || index >= Count)
            throw new IndexOutOfRangeException("The specified index is out of range.");
        var oldItem = _List[index];
        _List.RemoveAt(index);
        OnCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Remove, oldItem, index));
    }

    public void Add(T item)
    {
        _List.Add(item);
        OnCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Add, item));
    }

    public void Clear()
    {
        _List.Clear();
        OnCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Reset));
    }

    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 result = _List.Remove(item);
        OnCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Remove, item));
        return result;
    }

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

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

    int IList.Add(object value)
    {
        var result = ((IList)_List).Add(value);
        ;
        OnCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Add, value));
        return result;
    }

    bool IList.Contains(object value)
    {
        return ((IList)_List).Contains(value);
    }

    int IList.IndexOf(object value)
    {
        return ((IList)_List).IndexOf(value);
    }

    void IList.Insert(int index, object value)
    {
        ((IList)_List).Insert(index, value);
        OnCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Add, value, index));
    }

    void IList.Remove(object value)
    {
        ((IList)_List).Remove(value);
        OnCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Remove, value));
    }

    public void CopyTo(Array array, int index)
    {
        ((IList)_List).CopyTo(array, index);
    }
    #endregion

    #region Initialization
    public ObservableList()
    {
        _List = new List<T>();
    }

    public ObservableList(int capacity)
    {
        _List = new List<T>(capacity);
    }

    public ObservableList(IEnumerable<T> collection)
    {
        _List = new List<T>(collection);
    }
    #endregion
}

It seems to be working fine, but I have noticed an increase in processing time since switching to this implementation. Have I implemented the interfaces correctly?

Answer 1

1. The implementation of IList is inconsistent: You have implemented IsFixedSize explicitly while IsSynchronized and SyncRoot are not, yet they are all three part of the IList interface. You should probably implement IList entirely explicitly.
2. You occasionally do some unnecessary casting, like here ((IList)_List)[index]. Only cast if necessary (e.g. when casting the underlying List<T> into IList because you need access to some explicitly implemented interface methods/properties).

3. There is a potential race condition in your event raising method:

   private void OnCollectionChanged(NotifyCollectionChangedEventArgs args)
   {
       if (CollectionChanged != null)
           CollectionChanged(this, args);
   }
   

   If the subscriber unsubscribes after your if but before the actual call you will get a NullReferenceException. The idiomatic C# way of implementing this is:

   private void OnCollectionChanged(NotifyCollectionChangedEventArgs args)
   {
       var handler = CollectionChanged;
       if (handler != null)
           handler(this, args);
   }
   

   Note that this can still pose a problem on the subscriber side because it might get called just after it has unsubscribed but it should be dealt with there.

4. Calling event handlers happens synchronously. So if something has subscribed to the CollectionChanged event then the registered event handlers will be called synchronously on every operation raising the event. It is not totally surprising that this slows down the operation a bit. In the unobserved case however it should not make a huge difference.
5. To truly track down where the problem is you'll have to use a profiler.