9
\$\begingroup\$

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

    }
}
\$\endgroup\$
6
\$\begingroup\$

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";
\$\endgroup\$
  • 1
    \$\begingroup\$ I fail to see a single good reason to force inheritance in this case. As mentioned by ChrisW, this UndoManager can be implemented as separate and all-sufficient entity. And i think it should, otherwise its usefulness whould be quite limited. "Favor Aggregation over Inheritance" and blah blah :) \$\endgroup\$ – Nikita B Jan 14 '14 at 10:12
  • 1
    \$\begingroup\$ not necessary. You would still be able to implement Undo method on command level. Or implement undo by re-applying the command stack. \$\endgroup\$ – Nikita B Jan 14 '14 at 10:27
  • 1
    \$\begingroup\$ @Nik Sergey is saying that if StateManager is instance data instead of a base class, then the containing class needs to explicitly implement a visible method void RestoreState() { this.StateManager.RestoreState(); } instead of simply inheriting that method into its API. \$\endgroup\$ – ChrisW Jan 14 '14 at 10:56
  • 1
    \$\begingroup\$ If you don't like 'Manager' as a name then perhaps call it 'History'; and expose it as a public property of the class which contains (not subclasses) it, so that a client can could foo.History.Undo(). \$\endgroup\$ – ChrisW Jan 14 '14 at 13:58
  • 1
    \$\begingroup\$ @SergeyBerezovskiy - I have a follow up question. If Command holds both the undo and redo events, when a undo event takes place and is popped off the stack, you loose the associated redo event. Is is better to hold the events types in their own stack e.g. RedoStack and UndoStack? \$\endgroup\$ – PhillyNJ Jan 17 '14 at 15:20
7
\$\begingroup\$
  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.

\$\endgroup\$
5
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Chris, I like this approach. I still need to review all the answers (which its great how constructive the responses are). Putting your code online for review is like asking if your baby is ugly or pretty. Nevertheless, I think @Sergey's approach along with yours will improve my code. I'll work on it and post an edit for review. \$\endgroup\$ – PhillyNJ Jan 14 '14 at 13:51
  • 2
    \$\begingroup\$ I mostly agree with everything you wrote, but I would advise against using a struct to store command delegates. It's not only mutable and passed by value, but you don't get any options for inheritance, overriding stuff, proxying commands, or basically storing any state within the command itself. As a design decision at this time, it would be much better to keep your options open by creating an interface for a command, which can also easily have a "poor-man's" delegate-only implementation for quick and dirty commands. \$\endgroup\$ – Groo Jan 14 '14 at 14:21
  • 1
    \$\begingroup\$ @Groo Sorry about that. I meant it as pseudo-code for "A class with public members". \$\endgroup\$ – ChrisW Jan 14 '14 at 14:42
  • \$\begingroup\$ I've since read the answers/comments from on this post and did a major overhaul of the code. Should I start a new CR question to review the new code, of update my OP? \$\endgroup\$ – PhillyNJ Jan 16 '14 at 18:39
  • 1
    \$\begingroup\$ @PhilVallone See How to deal with follow-up questions? \$\endgroup\$ – ChrisW Jan 16 '14 at 19:00

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.