6
\$\begingroup\$

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
}
\$\endgroup\$
3
  • 1
    \$\begingroup\$ As a small info, given the ActionMap as a public property with a public setter is just asking for problems, for that having a method recovering the ActionWrapper should greatly reduce the risk there of user manipulation \$\endgroup\$
    – Icepickle
    Sep 5 '18 at 14:07
  • \$\begingroup\$ Thanks @Icepickle, please check latest Edit 1, it does take care of points you have raised \$\endgroup\$ Sep 6 '18 at 12:34
  • \$\begingroup\$ I have rolled back your last edit. Please see What should I do when someone answers my question?: Do not add an improved version of the code after receiving an answer. Including revised versions of the code makes the question confusing, especially if someone later reviews the newer code. \$\endgroup\$
    – t3chb0t
    Sep 6 '18 at 13:05
5
\$\begingroup\$

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));
    }

    ...
}
\$\endgroup\$
9
  • \$\begingroup\$ thanks for the brilliant set of design suggestions. Mine approach was first attempt and casual. I need to incorporate your suggestions, though the only difference that I have is user has an ability to decide to revert from an intermediary point, which means, that all actions are not undone but part actions are undone. There need to be a mechanism to define it , undo in part and commit the rest \$\endgroup\$ Sep 6 '18 at 6:24
  • \$\begingroup\$ Also there's a complete undo audit log, which shares the undo actions across various, ChangeTrackingList<T>, which needs to be sequentially undone, this would mean a Stack<Action> needs to be shared, in any case that's not type dependent even in your code \$\endgroup\$ Sep 6 '18 at 6:27
  • \$\begingroup\$ So if a user performs actions A, B and C, they should be able to undo B without having to undo C? That's going to be a lot more complicated. And if there are multiple lists (and probably more complex data structures and dependencies) then I think it's better to do change-tracking at a higher level. \$\endgroup\$ Sep 6 '18 at 7:39
  • \$\begingroup\$ Please check the modified design under Edit1, based on suggestions provided along with the use cases. There's a misunderstanding, if B is removed then C shall also be removed, all post changes shall be removed. However with new design it will be easier to even obtain what you have suggested, since a collection snapshot is taken at point in time (local), which can be shifted to global and intermediary changes can also be removed, post ensuring that its not already done as part of other changes \$\endgroup\$ Sep 6 '18 at 12:33
  • \$\begingroup\$ Your edit has been rolled back: if you want the updated code to be reviewed you can create a new question instead. Either way, I did take a look at it, and although it's an improvement in some areas, it still contains some major problems: bad use of static, not all methods being undoable, still publicly exposing state that should be kept private, interface still being too complex to use, still a lack of useful documentation, etc. \$\endgroup\$ Sep 6 '18 at 13:40

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.