Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm writing a WPF application and I have several ReadOnlyObservableCollection fields in my models.

Suppose that I wanted to create a FooViewModel instance for each FooModel instance. FooModel has an observable collection of BarModel, and FooViewModel should contain a (read-only) observable collection of BarViewModel.

I created an utility class to simplify this task:

using System;
using System.Collections;
using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.Collections.Specialized;
using System.Diagnostics.Contracts;
using System.Linq;

namespace NamespaceNameGoesHere
{
    /// <summary>
    /// Creates a view of another ("inner") read-only collection, using a function to map each instance in the source collection to another instance that is viewed by users of this class.
    /// If the inner collection uses the CollectionChanged event correctly, then the two are kept in sync.
    /// Note that when there are changes to the inner collection, some transformed instances might be recreated. Any attempt to prevent this should 
    /// This class is NOT thread-safe.
    /// </summary>
    /// <typeparam name="InnerType">The type of the instances in the inner collection</typeparam>
    /// <typeparam name="TargetType">The type of the instances that are exposed to users of this class</typeparam>
    public class TransformedCollection<InnerType, TargetType>
        : INotifyCollectionChanged, IReadOnlyCollection<TargetType>
    {
        public event NotifyCollectionChangedEventHandler CollectionChanged;

        private IReadOnlyCollection<InnerType> innerCollection;
        private List<TargetType> transformedValues = new List<TargetType>();
        private Func<InnerType, TargetType> transformationFunction;

        /// <summary>
        /// Creates a new transformed collection to view another collection.
        /// </summary>
        /// <param name="innerCollection">The collection containing the instances to transform. Must not be null.</param>
        /// <param name="transformationFunction">The function to convert an inner collection instance. Must not be null.
        /// This delegate must not throw any exceptions.</param>
        public TransformedCollection(IReadOnlyCollection<InnerType> innerCollection, Func<InnerType, TargetType> transformationFunction)
        {
            Contract.Requires<ArgumentNullException>(innerCollection != null, "innerCollection must not be null");
            Contract.Requires<ArgumentNullException>(transformationFunction != null, "transformationFunction must not be null");

            this.innerCollection = innerCollection;
            this.transformationFunction = transformationFunction;

            CreateCollection();

            //Ensure that when the inner collection is changed, the changes propagate to this one.
            ((INotifyCollectionChanged)innerCollection).CollectionChanged += InnerCollectionChanged;
        }

        /// <summary>
        /// Transforms and adds every element in the inner list
        /// </summary>
        private void CreateCollection()
        {
            foreach (var element in CreateTransformedElements())
            {
                transformedValues.Add(element);
            }
        }

        /// <summary>
        /// Transforms all elements in the inner list.
        /// </summary>
        /// <returns>An enumerable containing all the transformed elements.</returns>
        private IEnumerable<TargetType> CreateTransformedElements()
        {
            return innerCollection
                .Select(inner => transformationFunction(inner));
        }

        private void InnerCollectionChanged(object sender, NotifyCollectionChangedEventArgs e)
        {
            List<TargetType> newItems = null;
            List<TargetType> oldItems = null;

            switch (e.Action)
            {
                case NotifyCollectionChangedAction.Add:
                    newItems = new List<TargetType>();

                    for (int i = 0; i < e.NewItems.Count; ++i)
                    {
                        //Transform all newly added items and add them to temporary "newItems" list.

                        int index = e.NewStartingIndex + i;
                        InnerType innerElement = (InnerType)e.NewItems[i];

                        TargetType transformedElement = transformationFunction(innerElement);

                        newItems.Add(transformedElement);
                    }

                    transformedValues.InsertRange(e.NewStartingIndex, newItems);

                    RaiseCollectionChanged(this, new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Add, newItems, e.NewStartingIndex));

                    break;
                case NotifyCollectionChangedAction.Remove:
                    oldItems = new List<TargetType>();

                    for (int i = 0; i < e.OldItems.Count; ++i)
                    {
                        int index = e.OldStartingIndex + i;

                        oldItems.Add(transformedValues[index]);
                    }

                    transformedValues.RemoveRange(e.OldStartingIndex, e.OldItems.Count);

                    RaiseCollectionChanged(this, new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Remove, oldItems, e.OldStartingIndex));

                    break;
                case NotifyCollectionChangedAction.Replace:
                    newItems = new List<TargetType>();
                    oldItems = new List<TargetType>();

                    for (int i = 0; i < e.NewItems.Count; ++i)
                    {
                        int index = e.NewStartingIndex + i;
                        InnerType innerElement = (InnerType)e.NewItems[i];

                        TargetType transformedElement = transformationFunction(innerElement);

                        newItems.Add(transformedElement);
                        oldItems.Add(transformedValues[index]);

                        transformedValues[index] = transformedElement;
                    }

                    RaiseCollectionChanged(this, new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Replace, newItems, oldItems, e.NewStartingIndex));

                    break;
                case NotifyCollectionChangedAction.Reset:
                    transformedValues.Clear();

                    CreateCollection();

                    RaiseCollectionChanged(this, new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Reset));

                    break;
                case NotifyCollectionChangedAction.Move:
                    newItems = new List<TargetType>();

                    if (e.NewItems.Count != 1)
                    {
                        throw new NotImplementedException("No idea how this is supposed to work");
                    }

                    //The names "at old index"/"at new index" refer to the positions *before* the move method was called.
                    TargetType valueAtOldIndex = transformedValues[e.OldStartingIndex];
                    TargetType valueAtNewIndex = transformedValues[e.NewStartingIndex];

                    //Swap positions
                    transformedValues[e.NewStartingIndex] = valueAtOldIndex;
                    transformedValues[e.OldStartingIndex] = valueAtNewIndex;

                    //The value at the old index *before* the move method was called is the value that is at the new index *after* it.
                    //We add that value to the list
                    newItems.Add(valueAtOldIndex);

                    RaiseCollectionChanged(this, new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Move, newItems, e.NewStartingIndex, e.OldStartingIndex));

                    break;
                default:
                    throw new InvalidOperationException();
            }
        }

        private void RaiseCollectionChanged(object sender, NotifyCollectionChangedEventArgs e)
        {
            if (CollectionChanged != null)
            {
                CollectionChanged(sender, e);
            }
        }

        public IEnumerator<TargetType> GetEnumerator()
        {
            return transformedValues.GetEnumerator();
        }

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

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

NamespaceNameGoesHere is not the real name of the namespace.

I have created the following xUnit tests which all pass:

using System;
using System.Collections;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Collections.ObjectModel;
using Xunit;
using System.Collections.Specialized;

namespace NamespaceNameGoesHere.Test
{
    public class TransformedCollectionTest
    {
        private int Double(int x)
        {
            return x * 2;
        }

        [Fact]
        public void NullArgumentsTest()
        {
            var emptySourceCollection = new ObservableCollection<int>();
            var emptyCollection = new ReadOnlyObservableCollection<int>(emptySourceCollection);

            Assert.Throws<ArgumentNullException>(() => new TransformedCollection<int, int>(null, Double));
            Assert.Throws<ArgumentNullException>(() => new TransformedCollection<int, int>(emptyCollection, null));
            Assert.Throws<ArgumentNullException>(() => new TransformedCollection<int, int>(null, null));
        }

        [Fact]
        public void InitialStateTest()
        {
            var emptyCollection = new ObservableCollection<int>();
            var emptyReadOnlyCollection = new ReadOnlyObservableCollection<int>(emptyCollection);

            var emptyTransformCollection = new TransformedCollection<int, int>(emptyReadOnlyCollection, Double);

            //Collection must be empty
            Assert.Equal(0, emptyTransformCollection.Count);

            //Additionally, the enumerator (from IEnumerable<T>) will give no result
            Assert.Equal(false, emptyTransformCollection.GetEnumerator().MoveNext());

            //Check the same with the non-generic enumerator
            Assert.Equal(false, ((IEnumerable)emptyTransformCollection).GetEnumerator().MoveNext());

            var simpleCollection = new ObservableCollection<int>();
            simpleCollection.Add(1);
            simpleCollection.Add(2);
            simpleCollection.Add(3);

            var simpleReadOnlyCollection = new ReadOnlyObservableCollection<int>(simpleCollection);

            var simpleTransformCollection = new TransformedCollection<int, int>(simpleReadOnlyCollection, Double);

            Assert.Equal(3, simpleTransformCollection.Count);

            //We expect to get [2, 4, 6]
            int i = 2;
            foreach (int value in simpleTransformCollection)
            {
                Assert.Equal(i, value);

                i += 2;
            }
        }

        [Fact]
        public void AddElementTest()
        {
            var startIndices = new List<int>();
            var newItems = new List<IList>();

            var baseCollection = new ObservableCollection<int>();
            var readOnlyCollection = new ReadOnlyObservableCollection<int>(baseCollection);
            var transformedCollection = new TransformedCollection<int, int>(readOnlyCollection, Double);

            transformedCollection.CollectionChanged += (sender, args) =>
            {
                Assert.Same(transformedCollection, sender);

                Assert.Equal(NotifyCollectionChangedAction.Add, args.Action);

                //Keep track of the events
                startIndices.Add(args.NewStartingIndex);
                newItems.Add(args.NewItems);
            };

            Assert.Equal(0, startIndices.Count);

            baseCollection.Add(1);

            Assert.Equal(1, transformedCollection.Count);
            Assert.Equal(2, transformedCollection.First());

            //Check the events
            Assert.Equal(1, startIndices.Count);
            Assert.Equal(0, startIndices.First());
            Assert.Equal(1, newItems.First().Count);
            Assert.Equal(2, newItems.First()[0]);

            baseCollection.Add(2);

            Assert.Equal(2, transformedCollection.Count);
            Assert.Equal(2, transformedCollection.First());
            Assert.Equal(4, transformedCollection.ElementAt(1));

            //Check the events
            Assert.Equal(2, startIndices.Count);
            Assert.Equal(1, startIndices.ElementAt(1));
            Assert.Equal(1, newItems.ElementAt(1).Count);
            Assert.Equal(4, newItems.ElementAt(1)[0]);
        }

        [Fact]
        public void RemoveElementTest()
        {
            var startIndices = new List<int>();
            var oldItems = new List<IList>();

            var baseCollection = new ObservableCollection<int>();
            baseCollection.Add(1);
            baseCollection.Add(2);
            baseCollection.Add(3);

            var readOnlyCollection = new ReadOnlyObservableCollection<int>(baseCollection);
            var transformedCollection = new TransformedCollection<int, int>(readOnlyCollection, Double);

            transformedCollection.CollectionChanged += (sender, args) =>
            {
                Assert.Same(transformedCollection, sender);

                Assert.Equal(NotifyCollectionChangedAction.Remove, args.Action);

                //Keep track of the events
                startIndices.Add(args.OldStartingIndex);
                oldItems.Add(args.OldItems);
            };

            Assert.Equal(3, transformedCollection.Count);
            Assert.Equal(0, startIndices.Count);

            Assert.Equal(2, transformedCollection.First());
            Assert.Equal(4, transformedCollection.ElementAt(1));
            Assert.Equal(6, transformedCollection.ElementAt(2));


            baseCollection.RemoveAt(1);

            Assert.Equal(2, transformedCollection.Count);
            Assert.Equal(2, transformedCollection.First());
            Assert.Equal(6, transformedCollection.ElementAt(1));

            //Check the events
            Assert.Equal(1, startIndices.Count);
            Assert.Equal(1, startIndices[0]);
            Assert.Equal(1, oldItems.First().Count);
            Assert.Equal(4, oldItems.First()[0]);


            baseCollection.Remove(1);

            Assert.Equal(1, transformedCollection.Count);
            Assert.Equal(6, transformedCollection.First());

            //Check the events
            Assert.Equal(2, startIndices.Count);
            Assert.Equal(0, startIndices[1]);
            Assert.Equal(1, oldItems.ElementAt(1).Count);
            Assert.Equal(2, oldItems.ElementAt(1)[0]);


            baseCollection.RemoveAt(0);

            Assert.Equal(0, transformedCollection.Count);

            //Check the events
            Assert.Equal(3, startIndices.Count);
            Assert.Equal(0, startIndices[2]);
            Assert.Equal(1, oldItems.ElementAt(2).Count);
            Assert.Equal(6, oldItems.ElementAt(2)[0]);
        }

        [Fact]
        public void ClearCollectionTest()
        {
            var addStartIndices = new List<int>();
            var actions = new List<NotifyCollectionChangedAction>();

            var baseCollection = new ObservableCollection<int>();
            baseCollection.Add(1);
            baseCollection.Add(2);
            baseCollection.Add(3);

            var readOnlyCollection = new ReadOnlyObservableCollection<int>(baseCollection);
            var transformedCollection = new TransformedCollection<int, int>(readOnlyCollection, Double);

            transformedCollection.CollectionChanged += (sender, args) =>
            {
                Assert.Same(transformedCollection, sender);

                actions.Add(args.Action);

                switch (args.Action)
                {
                    case NotifyCollectionChangedAction.Add:
                        addStartIndices.Add(args.NewStartingIndex);

                        break;
                    case NotifyCollectionChangedAction.Reset:
                        //All we need to do is keep track that it happened, which we already do above
                        //(outside the switch statement)

                        break;
                    default:
                        Assert.True(false, "Unexpected event type: " + args.Action);
                        break;
                }
            };

            Assert.Equal(3, transformedCollection.Count);
            Assert.Equal(0, actions.Count);


            baseCollection.Clear();

            Assert.Equal(0, transformedCollection.Count);

            //Check the events
            Assert.Equal(1, actions.Count);
            Assert.Equal(NotifyCollectionChangedAction.Reset, actions[0]);


            baseCollection.Add(1);

            Assert.Equal(1, transformedCollection.Count);
            Assert.Equal(2, transformedCollection.ElementAt(0));

            //Check the events
            Assert.Equal(2, actions.Count);
            Assert.Equal(NotifyCollectionChangedAction.Add, actions[1]);
            Assert.Equal(1, addStartIndices.Count);
            Assert.Equal(0, addStartIndices[0]);


            baseCollection.Clear();

            Assert.Equal(0, transformedCollection.Count);

            //Check the events
            Assert.Equal(3, actions.Count);
            Assert.Equal(NotifyCollectionChangedAction.Reset, actions[2]);
        }

        [Fact]
        public void SetItemCollectionTest()
        {
            var oldStartIndices = new List<int>();
            var newStartIndices = new List<int>();
            var oldItems = new List<IList>();
            var newItems = new List<IList>();

            var baseCollection = new ObservableCollection<int>();
            baseCollection.Add(1);
            baseCollection.Add(2);
            baseCollection.Add(3);

            var readOnlyCollection = new ReadOnlyObservableCollection<int>(baseCollection);
            var transformedCollection = new TransformedCollection<int, int>(readOnlyCollection, Double);

            transformedCollection.CollectionChanged += (sender, args) =>
            {
                Assert.Same(transformedCollection, sender);

                Assert.Equal(NotifyCollectionChangedAction.Replace, args.Action);

                //Keep track of the events
                oldStartIndices.Add(args.OldStartingIndex);
                newStartIndices.Add(args.NewStartingIndex);
                oldItems.Add(args.OldItems);
                newItems.Add(args.NewItems);
            };

            Assert.Equal(0, oldStartIndices.Count);


            baseCollection[0] = 10;

            Assert.Equal(3, transformedCollection.Count);
            Assert.Equal(20, transformedCollection.ElementAt(0));
            Assert.Equal(4, transformedCollection.ElementAt(1));
            Assert.Equal(6, transformedCollection.ElementAt(2));

            //Check the events
            Assert.Equal(1, oldStartIndices.Count);
            Assert.Equal(0, oldStartIndices[0]);
            Assert.Equal(0, newStartIndices[0]);
            Assert.Equal(1, oldItems[0].Count);
            Assert.Equal(2, oldItems[0][0]);
            Assert.Equal(1, newItems[0].Count);
            Assert.Equal(20, newItems[0][0]);


            baseCollection[2] = 20;

            Assert.Equal(3, transformedCollection.Count);
            Assert.Equal(20, transformedCollection.ElementAt(0));
            Assert.Equal(4, transformedCollection.ElementAt(1));
            Assert.Equal(40, transformedCollection.ElementAt(2));

            //Check the events
            Assert.Equal(2, oldStartIndices.Count);
            Assert.Equal(2, oldStartIndices[1]);
            Assert.Equal(2, newStartIndices[1]);
            Assert.Equal(1, oldItems[1].Count);
            Assert.Equal(6, oldItems[1][0]);
            Assert.Equal(1, newItems[1].Count);
            Assert.Equal(40, newItems[1][0]);
        }

        [Fact]
        public void MoveElementTest()
        {
            var baseCollection = new ObservableCollection<int>();
            baseCollection.Add(1);
            baseCollection.Add(2);
            baseCollection.Add(3);
            baseCollection.Add(4);
            baseCollection.Add(5);

            var readOnlyCollection = new ReadOnlyObservableCollection<int>(baseCollection);

            var transformedCollection = new TransformedCollection<int, int>(readOnlyCollection, Double);

            int events = 0;

            transformedCollection.CollectionChanged += (sender, args) =>
            {
                events++;

                Assert.Equal(1, events);

                Assert.Same(transformedCollection, sender);

                Assert.Equal(1, args.NewItems.Count);
                Assert.Equal(1, args.OldItems.Count);

                Assert.Equal(4, args.NewItems[0]);
                Assert.Equal(4, args.OldItems[0]);

                Assert.Equal(2, args.NewStartingIndex);
                Assert.Equal(1, args.OldStartingIndex);
            };

            baseCollection.Move(1, 2);

            Assert.Equal(5, transformedCollection.Count);
            Assert.Equal(2, transformedCollection.ElementAt(0));
            Assert.Equal(6, transformedCollection.ElementAt(1));
            Assert.Equal(4, transformedCollection.ElementAt(2));
            Assert.Equal(8, transformedCollection.ElementAt(3));
            Assert.Equal(10, transformedCollection.ElementAt(4));
        }
    }
}

I am using only runtime code contract verifications (that is, the static analyzer is turned off).

I am currently satisfied with the current state of the code, but I want to see opinions from other programmers to cover any ground I may have missed. I am looking for reviews in the following areas:

  • Am I doing this right? That is:
    • Do you find this class useful?
    • Is there a built-in .NET class that already does this (or something very similar)?
  • Am I using IReadOnlyCollection and INotifyCollectionChanged right? Should I implement any other interfaces?
  • Am I doing unit tests right? Are they sufficiently comprehensive?
  • How is the code quality in general? Is it obvious what each part of the code does? Are the comments easy to understand?
  • Is the documentation of the class and public members (that aren't already documented in implemented interfaces) correct? Is it obvious how the class and its methods should be used?
  • Are the variable, method and class names appropriate?
  • Pretty much anything else you might find relevant.
share|improve this question

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Browse other questions tagged or ask your own question.