Problem
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
}
Solution
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 fromList<T>
: implement the necessary interfaces manually, and use an internalList<T>
for the actual storage. List<T>
(andIList<T>
) provides some other methods (Insert
,[int index]
andClear
) 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
orOriginalValue
? 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
andUndo
are much clearer thanActionWrapper
andAction
, andReplace
is a more accurate description of what theUpdate
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 thatActionMap
property: it should only be exposed as a get-onlyIReadOnlyDictionary
. - 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));
}
...
}