A custom Undo Manager

Question

I am working on building an UndoManager in C#. The concept is to store events and property changes in a Stack<Action<T>> instance. I believe it is working, but I am also looking for peer review.

What can I improve? Do you see any bugs?

public abstract class UndoManager<T>
{
    /// <summary>
    /// Adds an Event to Stack
    /// </summary>
    /// <param name="undoOperation"></param>
    protected abstract void Add(Action<T> undoOperation);

    /// <summary>
    /// Returns the size of the stack
    /// </summary>
    /// <returns></returns>
    public abstract int Size();
    /// <summary>
    /// Holds all the Events in a stack
    /// </summary>
    protected Stack<Action<T>> MyUndoOperations { get; set; }    
    protected Stack<Action<T>> MyRedoOperations { get; set; }    

    /// <summary>
    /// Preforms the undo action. Pops the last event and gets a reference to the next event 
    /// and fires the event 
    /// </summary>
    public abstract void Undo();    
    /// <summary>
    /// Preforms the Redo action.
    /// </summary>
    public abstract void Redo();       

}

public class MyClass : UndoManager<MyClass>
{
    #region Getter and Setters
    private String myValue;
    public String MyValue
    {
        get { return this.myValue; }

        set
        {

            this.Add(x => x.myValue = value);// save the operation on the stack               
            this.myValue = value;

        }
    }

    private String productName;
    public String ProductName
    {
        get { return productName; }

        set
        {
            this.Add(x => x.productName = value);// save the operation on the stack       
            productName = value;
        }
    }
    #endregion

    /// <summary>
    /// Returns the size of the Action Event Stack
    /// </summary>
    /// <returns>Size of this.MyOperations</returns>
    public override int Size()
    {
        return this.MyUndoOperations.Count();
    }

    public MyClass()
    {

        this.MyUndoOperations = new Stack<Action<MyClass>>();
        this.MyRedoOperations = new Stack<Action<MyClass>>();
    }
    /// <summary>
    /// Pushes the last Action event on the Stack
    /// </summary>
    /// <param name="undoOperation">Last Action Event</param>
    protected override void Add(Action<MyClass> undoOperation)
    {
        //this.MyRedoOperations.Clear();
        this.MyUndoOperations.Push(undoOperation);
    }

    /// <summary>
    /// Preforms the undo action. Pops the last event and gets a reference to the next event 
    /// and fires the event 
    /// </summary>
    public override void Undo()
    {

        if (this.MyUndoOperations != null && this.MyUndoOperations.Any())
        {

            Action<MyClass> topAction = this.MyUndoOperations.Pop();// remove the very last event.
            this.MyRedoOperations.Push(topAction); // add to the redo stack

            if (this.MyUndoOperations.Any())
            {
                // get a reference (peek) to the last event
                Action<MyClass> lastAction = this.MyUndoOperations.Peek();
                lastAction(this);// fire event  
                //this.Add(lastAction); // add to the undo stack
            }

        }

    }

    public override void Redo()
    {
        if (this.MyRedoOperations.Any())
        {

            Action<MyClass> lastAction = this.MyRedoOperations.Pop();
            lastAction(this);// fire event
            this.Add(lastAction);
        }

    }
}

Answer 1

In order to undo some changes, you should either do all changes besides last one, or you can remove last change and do opposite operation. E.g. if you are inserting something to database, then opposite operation will be removing from database.

Next, your class do to many things - it has own responsibilities, which are related to ProductName and MyValue managing. But you are also make this class managing actions stack - calculating size, adding and removing actions. I think all that behavior can and should be encapsulated in UndoManager class (btw, I don't like manager classes, because it's not clear what they do - it worth thinking about better name here).

So, common way to encapsulate operations is a Command pattern. Basic command which allows executing some operations (both direct and opposite) looks like:

public class Command        
{
    private readonly Action _action;
    private readonly Action _undoAction;        

    public Command(Action action, Action undoAction)
    {
        _undoAction = undoAction;
        _action = action;
    }

    public void Execute()
    {
        _action();
    }

    public void Undo()
    {
        _undoAction();
    }
}

Now, when we have operations encapsulated, we can create commands manager (I'm still thinking of better name):

public class StateManager
{
    private Stack<Command> commands = new Stack<Command>();

    protected void ChangeState(Command command)
    {
        command.Execute();
        commands.Push(command);
    }

    public void RestorePreviousState()
    {
        var command = commands.Pop();
        command.Undo();
    }

    // You can add Redo functionality here
}

It encapsulates working with commands. It executes command when you do something and it removes last command with undoing it when you want to undo last change. Now inherit from this class:

public class MyClass : StateManager
{
    private String myValue;
    private String productName;

    public String MyValue
    {
        get { return myValue; }
        set 
        {
            string currentValue = myValue;
            ChangeState(new Command(() => myValue = value, 
                                    () => myValue = currentValue)); 
        }
    }

    public String ProductName
    {
        get { return productName; }
        set
        {
            string currentValue = productName;
            ChangeState(new Command(() => productName = value, 
                                    () => productName = currentValue)); 
        }
    }
}

Now you can keep track on object state changes:

MyClass mc = new MyClass();
mc.MyValue = "foo";
// mc.MyValue is "foo";
mc.MyValue = "bar";
// mc.MyValue is "bar";
mc.RestorePreviousState();
// mc.MyValue is "foo";

Answer 2

1. What's most important, as @ChrisW wrote, you need two separate actions for Execute/Redo and Undo. A common command interface would be something like:

   public interface ICommand
   {
       // Executes this command.
       void Execute();
   
       // Undoes this command.
       void Undo();
   }
   

2. The UndoManager should not be generic nor abstract. By making it generic, you only allow it to store a limited set of commands, which is unnecessary. The manager should not know anything about the command it stores, apart from the fact that it can invoke Execute and Undo on them.

3. There is no need to make it abstract, either. I would however extract its interface so that you can test it (mock it), or provide dummy implementations in special cases. I doubt you would have additional derived undo managers, since its behavior is well defined, so I would probably limit the interface to something like:

   public interface IUndoManager
   {
       // Executes the specified commmand and adds it to the Undo stack.
       void Execute(ICommand commmand);
   
       // Undoes the last command in the Undo stack.
       void Undo();
   
       // Redoes the last command in the Redo stack.
       void Redo();
   }
   

   If you want to let your callers know more about the command manager state, then you can extend the whole thing by adding properties like Name to the ICommand, or CanUndo/CanRedo to the ICommandManager. That will allow your callers (i.e. a menu item in your Edit menu) to know if there exists an undoable command, and what's its description. But at all costs avoid exposing "internal" stuff, like the size of your stack.

4. Although I understand it's conceptual code, I would avoid prefix class names and property names with My (MyClass, MyUndoOperations). It doesn't provide any useful information.

Answer 3

Calling Add ought to clear the MyRedoOperations stack, but not when you call it from Redo.

Secondly, Action<T> is an uncomplicated type. It works for properties, because replaying an old action will reset the property to its previous value. However it won't work if you want a more general manager which can undo methods (not just properties), for example, the edit methods of a document. Consider a method which adds items to a list: if you replay an old method it will add the item again, not remove it. Therefore when I implemented an undo manager, instead of storing a single Action I would store a pair of actions (one for undo and one for redo), for example like the following pseudocode:

// A typical method which needs undoing
void InsertItem(object item)
{
    // this method should do:
    // this.MyList.Insert(item);

    // construct a pair of undo/redo actions
    EditorAction editorAction = new EditorAction() {
        Do = this.MyList.Insert(item),
        Undo = this.MyList.Remove(item)
    };
    // invoke the Do action and save it on the Undo stack
    NewAction(editorAction);
}

class EditorAction
{
    Action Undo;
    Action Do;
}

void NewAction(EditorAction editorAction)
{
    // Perform the action for the first time
    editorAction.Do();
    // Save the action on the undo stack
    UndoStack.Push(editorAction);
    RedoStack.Clear()
}

void UndoAction()
{
    EditorAction editorAction = UndoStack.Pop();
    editorAction.Undo();
    RedoStack.Push(editorAction);
}

void RedoAction()
{
    EditorAction editorAction = RedoStack.Pop();
    editorAction.Do();
    UndoStack.Push(editorAction);
}

Thirdly, beware of making UndoManager an abstract class. Do its Undo/Redo methods really need to be abstract and overriden, or could it be a concrete class instead? The problem with an abstract class is that C# doesn't support multiple inheritance: a class (e.g. MyClass) can only have one abstract base class; if UndoManager is an abstract class then it can't be used by any class which already has another base class.