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
add comment

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.