Observable Collection “Deep”

Question

In a project of mine I needed a class "Document" to be able to notify any change in its fields. This class keep a collection of class "AnotherClass". Using an ObservableCollection for this collection is not enough because my application can't receive notification from Document if a field INSIDE an instance of AnotherClass changes.

I wrote this implementation of "ObservableCollectionDeep" to overcome this problem:

   public class ObservableCollectionDeep<T> : ObservableCollection<T> where T : INotifyPropertyChanged
   {
        private bool _FreezeNotifications;
        private Action<object, PropertyChangedEventArgs> _CollectionItemChangedHandler;
        private Action<object, NotifyCollectionChangedEventArgs> _CollectionChangedHandler;

        public ObservableCollectionDeep(Action<object, NotifyCollectionChangedEventArgs> collectionChangedHandler,
                                    Action<object, PropertyChangedEventArgs> collectionItemChangedHandler)
            : base()
        {
            _CollectionChangedHandler = collectionChangedHandler;
            _CollectionItemChangedHandler = collectionItemChangedHandler;
        }

        public void AddRange(IEnumerable<T> collection)
        {
            _FreezeNotifications = true;
            collection.ToList().ForEach(p => Add(p));
            _FreezeNotifications = false;

            var eventArgs = new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Add, collection);

            onCollectionChanged(this, eventArgs);

            base.OnCollectionChanged(eventArgs);
        }

        public bool Replace(T element, T newElement)
        {
            if (this.Contains(element))
            {
                _FreezeNotifications = true;

                var index = this.IndexOf(element);
                this.Insert(index, newElement);
                this.Remove(element);

                _FreezeNotifications = false;

                var eventArgs = new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Replace, newElement, element);

                onCollectionChanged(this, eventArgs);

                base.OnCollectionChanged(eventArgs);

                return true;
            }

            return false;
        }

        [XmlIgnore]
        public new T this[int index]
        {
            get { return base[index]; }
            set { Replace(base[index], value); }
        }

        protected override void OnCollectionChanged(NotifyCollectionChangedEventArgs e)
        {
            handleNotificationsFromItemsInCollection(e);

            if (!_FreezeNotifications)
            {
                onCollectionChanged(this, e);

                base.OnCollectionChanged(e);
            }
        }

        private void handleNotificationsFromItemsInCollection(NotifyCollectionChangedEventArgs e)
        {
            switch (e.Action)
            {
                case NotifyCollectionChangedAction.Add:
                {
                    var elem = e.NewItems.Cast<INotifyPropertyChanged>();
                    if (elem != null)
                    {
                        elem.ToList().ForEach(p => p.PropertyChanged += onCollectionItemPropertyChanged);
                    }
                }
                break;

                case NotifyCollectionChangedAction.Move:
                break;

                case NotifyCollectionChangedAction.Remove:
                {
                    var elem = e.OldItems.Cast<INotifyPropertyChanged>();
                    if (elem != null)
                    {
                        elem.ToList().ForEach(p => p.PropertyChanged -= onCollectionItemPropertyChanged);
                    }
                }
                break;

                case NotifyCollectionChangedAction.Replace:
                {
                    var elem = e.OldItems.Cast<INotifyPropertyChanged>();
                    if (elem != null)
                    {
                        elem.ToList().ForEach(p => p.PropertyChanged -= onCollectionItemPropertyChanged);
                    }

                    elem = e.NewItems.Cast<INotifyPropertyChanged>();
                    if (elem != null)
                    {
                        elem.ToList().ForEach(p => p.PropertyChanged += onCollectionItemPropertyChanged);
                    }
                }
                break;

                case NotifyCollectionChangedAction.Reset:
                {
                    if (e.OldItems != null)
                    {
                        var elem = e.OldItems.Cast<INotifyPropertyChanged>();
                        if (elem != null)
                        {
                            elem.ToList().ForEach(p => p.PropertyChanged -= onCollectionItemPropertyChanged);
                        }
                    }
                }
                break;

                default:
                break;
            }
        }

        protected override void ClearItems()
        {
            this.ToList().ForEach(p => p.PropertyChanged -= onCollectionItemPropertyChanged);

            base.ClearItems();
        }

        protected virtual void onCollectionItemPropertyChanged(object sender, PropertyChangedEventArgs e)
        {
            _CollectionItemChangedHandler(sender, e);
        }

        protected virtual void onCollectionChanged(object sender, NotifyCollectionChangedEventArgs e)
        {
            _CollectionChangedHandler(sender, e);
        }
    }

    public static class ObservableCollectionDeepExtensionMethods
    {
        public static ObservableCollectionDeep<T> ToObservableCollectionDeep<T>(this IEnumerable<T> collection,
                                    Action<object, NotifyCollectionChangedEventArgs> collectionChangedHandler,
                                    Action<object, PropertyChangedEventArgs> collectionItemChangedHandler)
                                    where T : INotifyPropertyChanged
        {
            var c = new ObservableCollectionDeep<T>(collectionChangedHandler, collectionItemChangedHandler);
            c.AddRange(collection);

            return c;
        }
    }

Using ObservableCollectionDeep instead of ObservableCollection and "re-throwing" the notifications coming from this collection to the upper-level thanks to the two delegates, my application can subscribe to Document.PropertyChanged only and get notified for any changes.

I would like to have some feedback; Pros and Cons are welcome!

Thank you.

Answer 1

Design

You appear to be creating events without actually using events. Why? If it looks like an event, smells like an event and quacks like an event, call it an event.

So:

private Action<object, PropertyChangedEventArgs> _CollectionItemChangedHandler;

Should be:

public event EventHandler<PropertyChangedEventArgs> _CollectionItemChangedHandler;

I can see you wanting to use Action to enforce that users are subscribed to both events and prevent unsubscription later on, but I wonder why you require this? If a user wants to create your class and not subscribe to these events, what does it matter?

It is also recommended not to use .ToList().ForEach() in that way.

You shouldn't use ForEach to change objects. LINQ should be used in a "functional" way (you can create new objects but you can't change old objects nor you can create side-effects). And what you are writing is creating so many useless List only to gain two lines of code...

See this StackOverflow answer for more information.

Answer 2

It appears to me that you are working against and in spite of the ObservableCollection class. Not knowing your requirements I can't tell you exactly how to fix things but here is what I see wrong.

Use what .NET gives us here

1. @NickUdall is right. By using events as intended in the ObservableCollection design you get what I think you need.
2. INotifyPropertyChanged.PropertyChanged - this event is defined in ObservableCollection.
3. The above uses a PropertyChangedEventArgs object; and look what it has: a PropertyName property. i.e. the property (of the object) that changed.

Get rid of the "fixin' to" design

I see the class as "fixin' to" (as we say in the South) observe, but never doing so. I never read this OO principle in any book, it just feels like it fits here.

... a class "Document" to be able to notify any change in its fields. This class keep a collection of class "AnotherClass". Using an ObservableCollection for this collection ...

And, given this declaration:

public class ObservableCollectionDeep<T> : ObservableCollection<T>

I do not see Document "keeping" AnotherClass - I don't see a "has a" relationship, I see a "is a" relationship.

So <T> must be a AnotherClass, yes? So you are creating An ObservableCollection class of type AnotherClass which IS A ObservableCollection of type AnotherClass. This is the "fixin' to" flaw.

What to do?

Get rid of ObservableCollectionDeep<T>.

Instead have just-a-class class that has reference to and registers event handlers on an ObservableCollection object.

Here is a SO thread showing a simple example.