ViewModel base class supporting property notification, error notification and property value history

Question

I've created a general purpose ViewModel base class in what I believe it's purest behaviour. It features property notification, error notification and property value history. Please take a look at it and tell me what can be improved or what you think is missing.

For the sake of clarity I've merged everything in 1 codeblock:

namespace Trakt
{
 [System.AttributeUsage(AttributeTargets.Property, Inherited = false, AllowMultiple = false)]
 public sealed class TrackedAttribute : Attribute
 {
 public TrackedAttribute() { }
 }
 public abstract class ViewModelBase : INotifyPropertyChanged, INotifyDataErrorInfo
 {
 private const int MAXHISTORY = 10;
 private bool dirty;
 public bool Dirty
 {
 get { return dirty; }
 protected set { SetProperty(ref dirty, value); }
 }
 private bool corrupted;
 public bool Corrupted
 {
 get { return corrupted; }
 private set { SetProperty(ref corrupted, value); }
 }
 private readonly Guid _guid;
 private Stack<string> _changeList;
 private Dictionary<string, Stack<object>> _changes;
 private Dictionary<string, List<string>> _errors;
 public void ClearHistory()
 {
 _changeList = new Stack<string>();
 _changes = _changes
 .Select(_change => new KeyValuePair<string, Stack<object>>(_change.Key, new Stack<object>(new object[] { _change.Value.Pop() })))
 .ToDictionary(_change => _change.Key, _change => _change.Value);
 }
 public void ClearHistoryFor([CallerMemberName]string propertyName = "")
 {
 var lastValue = _changes[propertyName].Pop();
 _changeList = new Stack<string>(_changeList.Where(i => i != propertyName)); // changelist order still ok???
 _changes = _changes
 .Where(_change => _change.Key != propertyName)
 .Select(_change => new KeyValuePair<string, Stack<object>>(_change.Key, _change.Value))
 .ToDictionary(_change => _change.Key, _change => _change.Value);
 }
 public void ClearHistoryFor<TProperty>(Expression<Func<TProperty>> propertyExpression)
 {
 ClearHistoryFor(ExtractPropertyNameFor(propertyExpression));
 }
 protected ViewModelBase()
 {
 _guid = Guid.NewGuid();
 _changes = new Dictionary<string, Stack<object>>();
 _changeList = new Stack<string>();
 _errors = new Dictionary<string, List<string>>();
 var properties = from property in this.GetType().GetProperties()
 where Attribute.IsDefined(property, typeof(TrackedAttribute))
 select property;
 foreach (var property in properties)
 {
 //var trakt = property.GetCustomAttributes(typeof(TrackedAttribute), true);
 var value = this.GetType().GetProperty(property.Name).GetValue(this);
 _changes.Add(property.Name, new Stack<object>(new object[] { value }));
 }
 }
 public void Undo(int steps)
 {
 var count = _changes[_changeList.Peek()].Count;
 if (count <= 1)
 return;
 if (steps >= count)
 steps = count -1;
 var lastPropertyName = _changeList.Pop();
 var previousPropertyValue = _changes[lastPropertyName].Peek();
 int i = 0;
 do
 {
 previousPropertyValue = _changes[lastPropertyName].Pop();
 i++;
 } while (i <= steps);
 SetProperty(lastPropertyName, previousPropertyValue);
 }
 public void Undo()
 {
 Undo(1); 
 }
 protected bool IsTracked<TProperty>([CallerMemberName]string propertyName = "")
 {
 return _changes.ContainsKey(propertyName);
 }
 protected void Track<TProperty>([CallerMemberName]string propertyName = "")
 {
 Track(default(TProperty), propertyName);
 }
 protected void Track<TProperty>(TProperty value, [CallerMemberName]string propertyName = "")
 {
 if (_changes.ContainsKey(propertyName))
 return;
 if (!this.GetType().GetProperties().Select(p => p.Name).Contains(propertyName))
 return;
 _changes.Add(propertyName, new Stack<object>(new object[] { value }));
 }
 protected void UnTrack<TProperty>([CallerMemberName]string propertyName = "")
 {
 if (!_changes.ContainsKey(propertyName))
 return;
 _changes.Remove(propertyName);
 }
 #region INotifyDataErrorInfo
 public bool HasErrors { get { return Corrupted; } }
 public event EventHandler<DataErrorsChangedEventArgs> ErrorsChanged;
 private void OnErrorsChanged([CallerMemberName]string propertyName = "")
 {
 var handler = ErrorsChanged;
 handler?.Invoke(this, new DataErrorsChangedEventArgs(propertyName));
 }
 public IEnumerable GetErrors()
 {
 // TODO!
 throw new NotImplementedException();
 }
 public IEnumerable GetErrors<TProperty>(Expression<Func<TProperty>> propertyExpression)
 {
 return GetErrors(ExtractPropertyNameFor(propertyExpression));
 }
 public IEnumerable GetErrors([CallerMemberName]string propertyName = "")
 {
 var errors = new List<string>();
 _errors.TryGetValue(propertyName, out errors);
 return errors;
 }
 protected void AddError(string errorMessage, [CallerMemberName]string propertyName = "")
 {
 if (!_errors.ContainsKey(propertyName))
 _errors.Add(propertyName, new List<string>());
 _errors[propertyName].Add(errorMessage);
 OnErrorsChanged(propertyName);
 Corrupted = true;
 }
 protected void ClearError([CallerMemberName]string propertyName = "")
 {
 _errors.Remove(propertyName);
 OnErrorsChanged(propertyName);
 Corrupted = _errors.Select(e => e.Value).Count() != 0;
 }
 protected bool HasError([CallerMemberName]string propertyName = "")
 {
 return _errors.ContainsKey(propertyName) && _errors[propertyName].Count > 0;
 }
 #endregion
 #region INotifyPropertyChanged
 public event PropertyChangedEventHandler PropertyChanged;
 private void OnPropertyChanged([CallerMemberName]string propertyName = "")
 {
 var handler = PropertyChanged;
 handler?.Invoke(this, new PropertyChangedEventArgs(propertyName));
 }
 protected void SetProperty<TProperty>(string propertyName, TProperty value/*, bool notify = true*/)
 {
 this.GetType().GetProperty(propertyName).SetValue(this, value);
 }
 protected void SetProperty<TProperty>(ref TProperty field, TProperty value, [CallerMemberName]string propertyName = "")
 {
 if (!EqualityComparer<TProperty>.Default.Equals(field, value))
 {
 field = value;
 OnPropertyChanged(propertyName);
 if (_changes.ContainsKey(propertyName))
 {
 if(_changes[propertyName].Count == MAXHISTORY)
 {
 // max history limit reached, need to shift items...
 var list = new LinkedList<object>(_changes[propertyName]);
 list.RemoveLast();
 _changes[propertyName] = new Stack<object>(list.Reverse());
 }
 _changeList.Push(propertyName);
 _changes[propertyName].Push(value);
 }
 if(propertyName != nameof(Dirty))
 Dirty = true;
 }
 }
 protected void SetProperty<TProperty>(ref TProperty field, TProperty value, Expression<Func<TProperty>> propertyExpression)
 {
 SetProperty(ref field, value, ExtractPropertyNameFor(propertyExpression));
 }
 private static string ExtractPropertyNameFor<T>(Expression<Func<T>> propertyExpression)
 {
 var memberExp = propertyExpression.Body as MemberExpression;
 if (memberExp == null)
 throw new ArgumentException("Expression must be a MemberExpression.", "propertyExpression");
 return memberExp.Member.Name;
 }
 }
 #endregion
}

Sample implementation:

public class SomeViewModel: ViewModelBase
{
 private int trackedProperty;
 [Tracked]public int TrackedProperty
 {
 get { return trackedProperty; }
 set
 {
 SetProperty(ref trackedProperty, value, () => this.TrackedProperty);
 if (trackedProperty == 1)
 AddError("Value 1 is not allowed");
 else
 ClearError();
 }
 }
}

I'm very into SOLID (or try to) and clean code so don't be too gentle! :)

Answer 1

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!

• ʃʈɑɲ: Very helpful Jordan, I will look into this. I have already extended on the Viewmodel base class and still look to improve it so I'll take your notes in account!
• ʃʈɑɲ: I'll put this on Github when I completed the test assembly so you can collaborate if you want.