2
\$\begingroup\$

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.

\$\endgroup\$
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Nick, check the first variable name in the style section of your answer. "Quaks like an event" -> I love this \$\endgroup\$ – Heslacher Apr 22 '15 at 16:09
  • \$\begingroup\$ @Heslacher, indeed, although that refers to public fields. I now cannot find a source on correct casing for private fields so temporarily will remove that part. \$\endgroup\$ – Nick Udell Apr 22 '15 at 16:23
  • 1
    \$\begingroup\$ @NickUdell: None exists. Microsoft has purposely not standardized any conventions for private member names (even method names!), partly because they vary incredibly even within the source of .NET itself. \$\endgroup\$ – Magus Apr 22 '15 at 21:06
  • \$\begingroup\$ @Magus thanks for the info, that's good to know! \$\endgroup\$ – Nick Udell Apr 23 '15 at 8:09
  • \$\begingroup\$ @Magus: I did find this link: msdn.microsoft.com/en-us/library/ta31s3bc.aspx (third from the bottom) that says "Do not use uppercase letters for field names." but it seems a bit confusing what they're actually referring to there. \$\endgroup\$ – Nick Udell Apr 23 '15 at 8:18
1
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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