4
\$\begingroup\$

Description

In a time where thin clients, web browsers, web services and micro services are prevalent, we tend to forget we still have to deal with thick and smart clients. Many applications in B2B still use the latter. It is common in such clients that the user can perform a set of actions on the client before synchronizing to the server.

One such scenario is a master-detail view where the user can edit, cancel and apply changes on the master. On edit, a detail view is activated where the user can accept or cancel the changes for that particular edit. Each time the user accepts changes, they are considered the latest state of the entity. On a next edit, when cancelling, the state gets reverted to the last accepted state. Only when the master view is applied, all changes are pushed to the server. When the master view gets cancelled, all changes from the details are also reverted back to initial state.

API

Microsoft had a solution for this: Self-tracking Entities. But that API is no longer supported. Although other API's are available and/or being developed, I wanted to make an attempt at writing a low-level API as suggested by Microsoft: ..or writing custom code using the low-level change tracking APIs. I'm not sure I want to maintain or extend this API. I will probably first check whether the current version survives Code Review.

Current Features:

  • accept/reject changes to a complex object
  • can traverse object trees and cyclic graphs (hierarchical change tracking)
  • context-free change tracking that works on POCO
  • tracking a detail entity while the master is also being tracked independantly (composite change tracking)
  • stacking change trackers on a single entity (multi-level change tracking)

Currently Out-of-scope:

  • tracking items in collections
  • allowing not to track navigation properties

Questions

  • Does it make sense to make a custom API or should I use an existing one?
  • Is the API extensible, usable, maintainable?
  • Am I adhering to code conventions and best practices?

Code

The main object tracker for a consumer ObjectEditor allows for multi-level object tracker by implementing IEditableObject. BeginEdit starts an atomic update session. EndEdit accepts the current update, while CancelEdit reverts back to the last EndEdit or initial state. RejectChanges reverts back to initial state, regardless of number of edits that have been made, and AcceptChanges accepts all ongoing edits.

public sealed class ObjectEditor : IRevertibleChangeTracking, IEditableObject
{
    private readonly Stack<IRevertibleChangeTracking> trackers = new Stack<IRevertibleChangeTracking>();
    public bool IsChanged { get; private set; }
    public object Source { get; }

    public ObjectEditor(object source) => Source = source ?? throw new ArgumentNullException(nameof(source));

    public void AcceptChanges() => UntilEmpty(() => EndEdit());

    public void RejectChanges() => UntilEmpty(() => CancelEdit());

    public void BeginEdit() => trackers.Push(new ComplexObjectTracker(Source));

    public void CancelEdit() => Pop(tracker => tracker.RejectChanges());

    public void EndEdit() => Pop(tracker => tracker.AcceptChanges());

    private void UntilEmpty(Action operation)
    {
        while (trackers.Any()) operation();
    }

    private void Pop(Action<IRevertibleChangeTracking> operation)
    {
        if (!trackers.Any()) return;
        var tracker = trackers.Pop();
        operation(tracker);
    }
}

Internally, instances of ComplexObjectTracker are used to track entities, their properties and recursively track the entire object graph.

public class ComplexObjectTracker : IRevertibleChangeTracking
{
    public bool IsChanged { get; private set; }
    public object Source { get; }
    HashSet<object> Visited { get; }
    List<IRevertibleChangeTracking> PropertyTrackers { get; }

    public ComplexObjectTracker(object source, HashSet<object> visited = null)
    {
        Source = source ?? throw new ArgumentNullException(nameof(source));
        Visited = visited ?? new HashSet<object>();
        IsChanged = true;

        Visited.Add(Source);

        PropertyTrackers = (from p
              in source.GetType().GetProperties(BindingFlags.Public | BindingFlags.Instance)
                            where !p.GetIndexParameters().Any()
                            select p).Select(p => CreateChangeTracker(p)).ToList();
    }

    public void AcceptChanges() => Invoke(p => p.AcceptChanges());

    public void RejectChanges() => Invoke(p => p.RejectChanges());

    private void Invoke(Action<IRevertibleChangeTracking> operation)
    {
        PropertyTrackers.ForEach(operation);
        IsChanged = false;
    }

    private IRevertibleChangeTracking CreateChangeTracker(PropertyInfo property)
    {
        if (property.PropertyType.IsValueType)
        {
            return new PropertyReferenceTracker(Source, property);
        }

        return new DeepPropertyTracker(Source, property, Visited);
    }
}

As you can see, currently only two types of property tracking are implemented: PropertyReferenceTracker and DeepPropertyTracker. In future versions, additional trackers could be provided to track collections (shallow, deep, with ordering or not, etc).

public class PropertyReferenceTracker : IRevertibleChangeTracking
{
    public bool IsChanged { get; private set; }
    public object DeclaringInstance { get; }
    public object PropertyValue { get; }
    public PropertyInfo Property { get; }

    public PropertyReferenceTracker(object declaringInstance, PropertyInfo property)
    {
        DeclaringInstance = declaringInstance ?? throw new ArgumentNullException(nameof(declaringInstance));
        Property = property ?? throw new ArgumentNullException(nameof(property));
        PropertyValue = Property.GetValue(DeclaringInstance);
        IsChanged = true;
    }

    public virtual void AcceptChanges() => IsChanged = false;

    public virtual void RejectChanges()
    {
        if (Property.CanWrite)
        {
            Property.SetValue(DeclaringInstance, PropertyValue);
        }
        IsChanged = false;
    }
}

public class DeepPropertyTracker : PropertyReferenceTracker
{
    public IRevertibleChangeTracking deepTracker { get; }

    public DeepPropertyTracker(object declaringInstance, PropertyInfo property, HashSet<object> visited)
        : base(declaringInstance, property)
    {
        deepTracker = PropertyValue != null && !visited.Contains(PropertyValue) ? new ComplexObjectTracker(PropertyValue, visited) : null;
    }

    public override void AcceptChanges()
    {
        deepTracker?.AcceptChanges();
        base.AcceptChanges();
    }

    public override void RejectChanges()
    {
        deepTracker?.RejectChanges();
        base.RejectChanges();
    }
}

Usage Scenario

I made a small demo using a trivial POCO Employee to show the current features. As you can see, there is a graph of a potential infinite object depth. This should show how object graphs are handled.

class Employee
{
    public DateTime BirthDate { get; set; }
    public string FirstName { get; set; }
    public string Name { get; set; }
    public Employee Manager { get; set; }
}

And some unit tests to verify the behavior of change tracking.

[TestClass]
public class Fixtures
{
    private Employee employee;

    [TestInitialize]
    public void TestInit()
    {
        var manager = new Employee
        {
            BirthDate = new DateTime(1980, 1, 1),
            FirstName = "John",
            Name = "Doe"
        };

        employee = new Employee
        {
            BirthDate = new DateTime(2000, 1, 1),
            FirstName = "Rembrandth",
            Name = "Smith",
            Manager = manager
        };
    }

    [TestMethod]
    public void EndEdit()
    {
        var editor = new ObjectEditor(employee);
        editor.BeginEdit();
        Assert.AreEqual(employee.Name, "Smith");

        employee.Name = "X";
        editor.EndEdit();
        Assert.AreEqual(employee.Name, "X");
    }

    [TestMethod]
    public void CancelEdit()
    {
        var editor = new ObjectEditor(employee);
        editor.BeginEdit();
        Assert.AreEqual(employee.Name, "Smith");

        employee.Name = "X";
        editor.CancelEdit();
        Assert.AreEqual(employee.Name, "Smith");
    }

    [TestMethod]
    public void EndHierarchicalEdit()
    {
        var editor = new ObjectEditor(employee);
        editor.BeginEdit();
        Assert.AreEqual(employee.Manager.Name, "Doe");

        employee.Manager.Name = "X";
        editor.EndEdit();
        Assert.AreEqual(employee.Manager.Name, "X");
    }

    [TestMethod]
    public void EndHierarchicalEdit_OtherRef()
    {
        var editor = new ObjectEditor(employee);
        editor.BeginEdit();
        Assert.AreEqual(employee.Manager.Name, "Doe");

        employee.Manager = new Employee
        {
            BirthDate = new DateTime(1789, 1, 1),
            FirstName = "Juan",
            Name = "Carlos"
        };
        editor.EndEdit();
        Assert.AreEqual(employee.Manager.Name, "Carlos");
    }

    [TestMethod]
    public void CancelHierarchicalEdit_OtherRef()
    {
        var editor = new ObjectEditor(employee);
        editor.BeginEdit();
        Assert.AreEqual(employee.Manager.Name, "Doe");

        employee.Manager = new Employee
        {
            BirthDate = new DateTime(1789, 1, 1),
            FirstName = "Juan",
            Name = "Carlos"
        };
        editor.CancelEdit();
        Assert.AreEqual(employee.Manager.Name, "Doe");
    }

    [TestMethod]
    public void CancelHierarchicalEdit()
    {
        var editor = new ObjectEditor(employee);
        editor.BeginEdit();
        Assert.AreEqual(employee.Manager.Name, "Doe");

        employee.Manager.Name = "X";
        editor.CancelEdit();
        Assert.AreEqual(employee.Manager.Name, "Doe");
    }

    [TestMethod]
    public void EndCompositeEdit()
    {
        var editor = new ObjectEditor(employee);
        editor.BeginEdit();
        Assert.AreEqual(employee.Manager.Name, "Doe");

        employee.Manager.Name = "X";

        {
            var compositeEditor = new ObjectEditor(employee.Manager);
            compositeEditor.BeginEdit();

            Assert.AreEqual(employee.Manager.Name, "X");

            employee.Manager.Name = "Y";

            compositeEditor.EndEdit();
            Assert.AreEqual(employee.Manager.Name, "Y");
        }

        editor.EndEdit();
        Assert.AreEqual(employee.Manager.Name, "Y");
    }

    [TestMethod]
    public void CancelCompositeEdit()
    {
        var editor = new ObjectEditor(employee);
        editor.BeginEdit();
        Assert.AreEqual(employee.Manager.Name, "Doe");

        employee.Manager.Name = "X";

        {
            var compositeEditor = new ObjectEditor(employee.Manager);
            compositeEditor.BeginEdit();

            Assert.AreEqual(employee.Manager.Name, "X");

            employee.Manager.Name = "Y";

            compositeEditor.EndEdit();
            Assert.AreEqual(employee.Manager.Name, "Y");
        }

        editor.CancelEdit();
        Assert.AreEqual(employee.Manager.Name, "Doe");
    }
}
\$\endgroup\$
  • \$\begingroup\$ Comments are not for extended discussion; this conversation has been moved to chat. \$\endgroup\$ – Vogel612 Aug 24 at 21:16
2
\$\begingroup\$

From where I'm standing it looks like this...

usability

  • BeginEdit & EndEdit seem to be unsafe in use. What happens when I call either of them multiple times? Do they have to match? I mean, do I have to call EndEdit as many times as I called BeginEdit? Probably yes, but the API isn't clear about that. Or what would happen when I call EndEdit without BeginEdit? These scenarios should not be possible. What I think would be more intuitive and also reliable is this:

    • let BeginEdit return a new interface

      public interface IEditing : IDisposabe, IRevertibleChangeTracking {}
      
    • this can used with using or any other disposable session mechanism

      using(var editing = objectEditor.BeginEdit()) // <-- IEditing
      {
          // make changes...
      }
      
    • throw an InvalidOperationException when BeginEdit is called multiple times for an object

    • Dispose would end the session and would also throw the same exception when there are any uncommited changes. This would be similar to an application warning you about unsaved changes when you are closing the editor. Of course the user should have other APIs to prevent that. The exception would be the last resort warning.
    • let the user only have the original APIs from the IRevertibleChangeTracking. The pair Begin/EndEdit are too confusing.
  • and there is also a question: what is going to happen when I try to edit an object with two editors (UIs) at the same time? Should this be possible or prohibited? How will conflicts be resolved?
  • you are using a Stack for tracking changes internally. I guess this means that BeginEdit & EndEdit work like QuickSave and Undo. Again, I think it would be more intuitive if there were actually such methods as TakeSnapshot and Undo. This however would require the use to be able to see how many snapshots there are (as a counter or ObjectEditor could itself be IEnumerable<OfSomething>) and when he tries to Undo more, then InvalidOperationException should be raised.

clean-code

  • some of your members have access modifiers whereas others don't and there is no pattern ;-P

  • I would call ComplexObjectTracker just ObjectTracker. No need to confuse the user that there is anything complex. Especially that there is no ObjectTracker yet so why is this one complex?

  • I also think that all classes but the ObjectEditor should be internal and the user should see only the interfaces.

  • there is the PropertyReferenceTracker but the derived class is named DeepPropertyTracker and not DeepPropertyReferenceTracker. I would remove the word Reference from the name. It's confusing. See here where a PropertyReferenceTracker is created for a value-type and a deep-tracker for a reference type:

    private IRevertibleChangeTracking CreateChangeTracker(PropertyInfo property)
    {
     if (property.PropertyType.IsValueType)
     {
         return new PropertyReferenceTracker(Source, property);
     }
    
     return new DeepPropertyTracker(Source, property, Visited);
     }
    
  • Are you sure this should even be public? And the name...

    public IRevertibleChangeTracking deepTracker { get; }
    

misc

  • I don't think that setting IsChanged = true; for all properties, regardless whether they are actually changed or not is such a good idea. This would make it only more difficult to view the changes... that might actually not be there. It would also unnecessarily overwrite unmodified values. Having such helpers as OldValue and NewValue would also be very useful for rendering debug views or other summaries.
\$\endgroup\$
  • 1
    \$\begingroup\$ I'll edit the answer when I find anything else... \$\endgroup\$ – t3chb0t Aug 25 at 6:13
  • \$\begingroup\$ what is going to happen when I try to edit an object with two editors (UIs) at the same time? last one wins, but this is not clear from my spec \$\endgroup\$ – dfhwze Aug 25 at 7:20
  • \$\begingroup\$ What happens when I call either of them multiple times? Do they have to match? it's possible to call them multiple times, it's like the matching parenthesis problem. You can nest edits (like a stack). It's also a sandbox, no exception when called if no matching end/begin pattern. perhaps throwing the exception as you suggest is a safer alternative \$\endgroup\$ – dfhwze Aug 25 at 7:22

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.