Undo Framework Design (Revert the changes in the collection)

Question

Requirement is to monitor the changes in a List<T>, possible changes are Add / Remove / Update, which are registered in an Audit log, which I do in the code underneath using a Dictionary. Now user can take an action to revert each of the operations, using an integrated Action delegate. Each operation runs its respective Revert operation. Please note here undo is not about notification, like ObservableCollection<T> but removal at the later time based on user discretion. Following is my design, please suggest, what shall be done to further improvise.

---

public class ActionWrapper<T>
{
    public int Index {get;set;}
    public T OriginalValue {get;set;}
    public T NewValue {get;set;}
    public Action<int,T> Action {get;set;}  
}

public class ChangeList<T> : List<T> 
                             where T:class,IEquatable<T>
{
    public Dictionary<T,ActionWrapper<T>> ActionMap {get;set;}

    public ChangeList()
    {
        ActionMap = new Dictionary<T,ActionWrapper<T>>();
    }

    public new void Add(T item)
    {
        base.Add(item); 

        var actionWrapper = new ActionWrapper<T>
        {
            Action = new Action<int,T>(RevertAdd),
            Index = this.FindIndex(x => x.Equals(item)),
            NewValue = item,
            OriginalValue = null
        };

        ActionMap[actionWrapper.NewValue] = actionWrapper;
    }

    public new void Remove(T item)
    {   
        var actionWrapper = new ActionWrapper<T>
        {
            Action = new Action<int, T>(RevertRemove),
            Index = this.FindIndex(x => x.Equals(item)),
            NewValue = null,
            OriginalValue = item
        };

        if(actionWrapper.Index < 0)
           return;

        base.Remove(actionWrapper.OriginalValue);

        ActionMap[actionWrapper.OriginalValue] = actionWrapper;
    }

    public void Update(T actualValue,T newValue)
    {
        var actionWrapper = new ActionWrapper<T>
        {
            Action = new Action<int, T>(RevertUpdate),
            Index = this.FindIndex(x => x.Equals(actualValue)),
            NewValue = newValue,
            OriginalValue = actualValue
        };

        if (actionWrapper.Index < 0)
            return;

        base[actionWrapper.Index] = newValue;

        ActionMap[actionWrapper.NewValue] = actionWrapper;
    }

    public void RevertAdd(int index, T actual)
    {
        base.Remove(actual);    
    }

    public void RevertRemove(int index,T actual)
    {
        base.Add(actual);
    }

    public void RevertUpdate(int index,T actual)
    {
        base[index] = actual;
    }
}

Use Case

void Main()
{
    var changeList = new ChangeList<string>();
    changeList.Add("Person1");
    changeList.Add("Person2");
    changeList.Add("Person3");
    changeList.Add("Person4");
    changeList.Add("Person5");
    changeList.Add("Person6");
    changeList.Add("Person7");
    changeList.Dump(); // Print statement

    var actionMapUpdateAdd = changeList.ActionMap["Person5"];
    actionMapUpdateAdd.Action(actionMapUpdateAdd.Index, actionMapUpdateAdd.NewValue);
    changeList.Dump(); // Print statement

    changeList.Update("Person7","Person77");
    changeList.Dump(); // Print statement

    var actionMapUpdate = changeList.ActionMap["Person77"];
    actionMapUpdate.Action(actionMapUpdate.Index,actionMapUpdate.OriginalValue);     
    changeList.Dump(); // Print statement

    changeList.Remove("Person6");
    changeList.Dump(); // Print statement

    var actionMapRemove = changeList.ActionMap["Person6"];
    actionMapRemove.Action(actionMapRemove.Index, actionMapRemove.OriginalValue);
    changeList.Dump(); // Print statement
}

Answer 1

Edge cases

This fails to handle several edge-cases:

• Undoing a remove action multiple times results in that item being added back multiple times.
• Undoing an update action after other items have been inserted at a lower index causes the wrong items to be replaced.
• Lists can contain duplicate items, but only the last operation for each distinct item is remembered.
• Updating an item leaves the add-operation for the original item, but attempting to undo that add-operation fails unless the update action has first been undone.

As the last point demonstrates, you can't just undo an action without undoing all actions that followed it first. If you do want to support something like that, you'll have to clearly define the requirements and figure out what the desired behavior is for a variety of edge-cases. You'll also want to make this information available to those that will use this code (documentation, see below).

Implementation notes

• Hiding methods with new is rarely a good idea: (changeList as IList<string>).Add("untracked item"); probably does not do what you want it to do. In this case, don't inherit from List<T>: implement the necessary interfaces manually, and use an internal List<T> for the actual storage.
• List<T> (and IList<T>) provides some other methods (Insert, [int index] and Clear) that are not being 'intercepted', resulting in untracked changes.
• Undoing an action is complicated. Why should the caller need to know whether to use NewValue or OriginalValue? That makes it difficult to use correctly. Why does the caller need to pass in any arguments at all? Why use a wrapper class if you can just create a closure with all the necessary state?
• Try to use clear, descriptive names. UndoableAction and Undo are much clearer than ActionWrapper and Action, and Replace is a more accurate description of what the Update method does.
• Those RevertAdd/Remove/Update methods don't seem to be intended for public use, so don't make them public. They only clutter the interface of your class.
• Those ActionWrapper properties should probably not be public either, but if they have to be, then at least make them read-only. You don't want other code to be able to mess with the internals of your change-tracking/undo system. The same goes for that ActionMap property: it should only be exposed as a get-only IReadOnlyDictionary.
• Documentation is entirely absent. That makes it even more difficult to tell how this class is meant to be used (or even what its exact purpose is), and various details such as Remove only removing the first matching item are left to the caller to figure out. It also makes it difficult for others to distinguish between intended and incorrect behavior.

Alternative design

I'd go for a different design, one that doesn't expose internal details, doesn't allow for out-of-order undoing (which means less edge-cases), and that provides a simple interface that's easy to use correctly (note how it's not possible to undo the same action multiple times):

public class ChangeTrackingList<T> // implements IList<T> and/or other interfaces
{
    private List<T> _items = new List<T>();
    private Stack<Action> _undoActions = new Stack<Action>();

    public bool UndoLastAction()
    {
        if (!_undoActions.Any())
            return false;

        var undoLastAction = _undoActions.Pop();
        undoLastAction();
        return true;
    }

    public void Add(T item)
    {
        _items.Add(item);

        // Ensure that this item gets removed, and not an identical earlier occurrence:
        var index = _items.Count - 1;
        _undoActions.Push(() => _items.RemoveAt(index));
    }

    ...
}