Return to Answer

• 1

• 198
• 8

This is a pretty cool implementation. It's fairly clean and usable. Just a few things standout to me:

In SetProperty<TProperty>(ref TProperty field, TProperty value, [CallerMemberName]string propertyName = "") it's a bit of a red flag to see the conversion to a LinkedList and then back to a Stack<T> in order to pop the first item at the 'bottom' of the Stack<T>. Stack<T> isn't really the ideal type of data structure if you find yourself needing to do this. You may want to consider just using a LinkedList<T> instead of Stack<T>.

LinkedList<T> offers methods like AddFirst, AddLast as well as RemoveLast and RemoveFirst. The below comments could apply to usage of a LinkedList<T> as well.

In the ViewModelBase ctor() you get back all the properties with the TrackedAttribute on them then add their initial values in a subsequent for loop. For each of the properties in the properties enumerable you reflect on the current type to get the PropertyInfo object so you can get the value, but you already have the PropertyInfo object from the linq above it.

Changing this to property.GetValue(this) should save you the extra reflection call per property.

If you wanted to you should be able to eliminate the foreach loop entirely with a little extra linq:

var properties = (from property in GetType().GetProperties()
 where Attribute.IsDefined(property, typeof(TrackedAttribute))
 select property).ToDictionary(prop => prop.Name, 
 prop => new Stack<object>(new[] { prop.GetValue(this) }));

You might want to consider handling an empty Stack in case someone attempts to Undo() when the changeList Stack<T> is empty on var count = _changes[_changeList.Peek()].Count;. According to msdn an InvalidOperationException will be thrown when calling Peek() on a Stack with no objects on it. I believe a possible unit test for this is calling Undo() once then calling it again (Call twice since each 'Tracked' object has a Stack<T> created with its initial value).

Additionally, changing the name of Undo() to something a bit more descriptive would be helpful. E.g., UndoPrevious() or UndoLast(). This way the consumer knows right away just how much 'undoing' this method does.

Where is the _guid member used? I see where it is created in the ctor, but not consumed.

Consider catching/handling the Pop() call for an empty Stack in ClearHistoryFor() as well (although it does not appear lastValue is used).

I'm not entirely certain what the benefit of the Expression<Func<TProperty>> propertyExpression overloads of SetProperty are over using the CallerMemberNameAttribute since CallerMemberNameAttribute will send in the name of the property for you at runtime. Getting the property name from the Expression<Func<T>> is slower than from CallerMemberNameAttribute. I'm assuming you are using C#5 (or 6) based on the nameof() usage to check if the property name is 'Dirty'. Marc Gravell details this here if interested.

In answer to your comment/question about "changelist order still ok???" I believe it will be in order. Here are some sources for my reasoning: JSkeet on preserving order with linq and Microsoft's albeit partially vague statement on the topic.

Relevant section from that article (near the top):

In PLINQ, the goal is to maximize performance while maintaining correctness. A query should run as fast as possible but still produce the correct results. In some cases, correctness requires the order of the source sequence to be preserved; however, ordering can be computationally expensive. Therefore, by default, PLINQ does not preserve the order of the source sequence. In this regard, PLINQ resembles LINQ to SQL, but is unlike LINQ to Objects, which does preserve ordering.

Hope this helps!