Implementing IList<T>, IList & INotifyCollectionChanged

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 : IList, IList, INotifyCollectionChanged
{
public event NotifyCollectionChangedEventHandler CollectionChanged;

#region Properties
private List _List
{
get;
set;
}

public T this[int index]
{
get
{
return _List[index];
}
set
{
if (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)_List).IsReadOnly;
}
}

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

object IList.this[int index]
{
get
{
return ((IList)_List)[index];
}
set
{
if (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));
}

• 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.

• You occasionally do some unnecessary casting, like here ((IList)_List)[index]. Only cast if necessary (e.g. when casting the underlying List into IList because you need access to some explicitly implemented interface methods/properties).

-
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.

• 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.

• To truly track down where the problem is you'll have to use a profiler.

StackExchange Code Review Q#30708, answer score: 8