HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

A custom Undo Manager

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
managercustomundo

Problem

I am working on building an UndoManager in C#. The concept is to store events and property changes in a Stack> 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
{
///
/// Adds an Event to Stack
///
///
protected abstract void Add(Action undoOperation);

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

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

}

public class MyClass : UndoManager
{
#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

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

public MyClass()
{

this.MyUndoOperations = new Stack>();
this.MyRedoOperations = new Stack>();
}
///
/// Pushes the last Action event on the Stack
///
/// Last Action Event
protected override void Add(Action

Solution

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


-
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.

-
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.

-
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.

Code Snippets

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

    // Undoes this command.
    void Undo();
}
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();
}

Context

StackExchange Code Review Q#39186, answer score: 7

Revisions (0)

No revisions yet.