3
\$\begingroup\$

This is a follow up review of question 'a custom Undo Manager'.

After reviewing all the comments and answers, my code has been revised (completely rewritten) based on the review.

Summary

The intent of the code is to provide a means to keep track of events in c#. Each Action has an undo and redo event (Command) and is pushed onto a redo or undo Stack<Command>. An object can create an instance of the StateManager, which should be private in order keep the undo and redo events encapsulated. For example purposes, I created a class called Car, which has a property of type StateManager which handles all the events. Your comments and suggestions are appreciated.

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

    // Undoes this command.
    void Undo();
}

/// <summary>
/// The Command class holds a reference to an Event and the Undo Event
/// </summary>
public class Command:ICommand
{
    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();
    }

}

public interface IStateManager
{
    // Executes the specified commmand and adds it to the Undo stack.
    void ChangeState(Command commmand);

    // Undoes the last command in the Undo stack.
    void RestorePreviousState();

    // Redoes the last command in the Redo stack.
    void RedoPreviousState();
}

public class StateManager:IStateManager
{
    private Stack<Command> _undoCommands = new Stack<Command>();
    private Stack<Command> _redoCommands = new Stack<Command>();


    public void ChangeState(Command command)
    {
        command.Execute();
        _undoCommands.Push(command);
    }

    public void RestorePreviousState()
    {
        if (this._undoCommands.Any()) {

            var command = _undoCommands.Pop();
            command.Undo(); 
            _redoCommands.Clear();// clear the redo stack
            _redoCommands.Push(command);// push event on redo stack
        }              

    }

    public void RedoPreviousState() {

        if (this._redoCommands.Any())
        {
            var commandPop = this._redoCommands.Pop();
            commandPop.Execute();
            this._undoCommands.Push(commandPop);  // put the event on the undo stack                             

        }
    }       
}

public class Car
{
    /// <summary>
    /// StateManager - manages the state of properties/methods that call  StateManager.ChangeState.
    /// The StateManager is set to private for encapsulated - The State of the object is only handled 
    /// by the object that instantiates it.
    /// </summary>
    private StateManager _manager { get; set; }

    private String color;

    public String Color
    {
        get { return color; }
        set {               
            string currentValue = color;
            _manager.ChangeState(new Command(() => color = value,
                                                    () => color = currentValue)); 
        }   
    }
    public Car()
    {
        _manager = new StateManager();
    }

    /// <summary>
    /// Undo the last event. 
    /// </summary>
    public void Undo() {

        _manager.RestorePreviousState();
    }

    /// <summary>
    /// Redo the last event
    /// </summary>
    public void Redo()
    {
        _manager.RedoPreviousState();
    }      

}
\$\endgroup\$
3
\$\begingroup\$

I think you should not clear _redoCommands stack when you are re-doing command. Actually you should be able to continue re-doing command until you will restore state when you called first undo command. You should clear redo-commands when completely new command arrives:

public void ChangeState(Command command)
{
    command.Execute();
    _undoCommands.Push(command);
    _redoCommands.Clear(); // new command arrived, you cannot redo anymore
}

public void RestorePreviousState()
{
    if (!_undoCommands.Any())
        return;

    var command = _undoCommands.Pop();
    command.Undo();
    _redoCommands.Push(command);      
}

public void RedoPreviousState()
{
    if (!_redoCommands.Any())
       return;

    var command = _redoCommands.Pop(); // commandPop a little strange name :)
    command.Execute();
    _undoCommands.Push(command);        
}

Also I'm thinking on improving naming... because RestorePreviousState() looks like restoring state which was before last state change - i.e. two call of this method should leave object in initial state.

UPDATE: I'd go with UndoStateChange and RedoStateChange without word previous - I find such terms less confusing. Also I'd think about making public properties for checking available state changes. Because currently it's not clear whether state changed or not. You call UndoStateChange and nothing is returned whether state changed or there is nothing to undo.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ 500 points! Congratulations! \$\endgroup\$ – Mathieu Guindon Jan 18 '14 at 20:13
  • \$\begingroup\$ @retailcoder thanks :) Nice site, which needs more attention I think (and small design refactoring) :) \$\endgroup\$ – Sergey Berezovskiy Jan 18 '14 at 20:16
  • \$\begingroup\$ The design is because we're still a beta site - we're trying real hard to make 2014 our graduation year! Seen this? \$\endgroup\$ – Mathieu Guindon Jan 18 '14 at 20:18
  • 1
    \$\begingroup\$ @retailcoder nope, I haven't seen that, thanks for link :) Stack Exchange Data Explorer also seems interesting! \$\endgroup\$ – Sergey Berezovskiy Jan 18 '14 at 20:23
  • 1
    \$\begingroup\$ If, like us (the <s>fanatics</s> regulars), you want CR to graduate soon, make sure you check out The 2nd Monitor, this site's main chatroom. Weekends aren't especially busy, but there's always someone in there. And it'll get busier when we resume with weekend-challenge :) \$\endgroup\$ – Mathieu Guindon Jan 18 '14 at 20:30
2
\$\begingroup\$

The code looks good (apart from _redoCommands.Clear() in the wrong place) but could be a little bit shorter:

  • Remove the ICommand and IStateManager interfaces (no need for them)
  • Making the Action instances public properties of Command would mean you could invoke them directly
  • Making StateManager data a public property of Car would mean you could invoke its methods directly ... and returning a new IStateHistory interface would let you let you hide one of the StateManager methods (i.e. the ChangeState method which should only be called from within the Car implementation

For example (altered code, with all your comments removed and my comments added):

public class Command
{
    // public get and private set is similar to readonly
    public Action Execute { get; private set; }
    public Action Undo { get; private set; }
    public Command(Action action, Action undoAction)
    {
        this.Execute = action;
        this.Undo = undoAction;
    }
}

// this interface doesn't expose the ChangeState method
public interface IStateHistory
{
    void RestorePreviousState();
    void RedoPreviousState();
}

// The ChangeState method isn't in the IStateHistory interface,
// and can be internal instead of public.
public class StateManager : IStateHistory
{
    ... methods of StateManager are as shown in Sergey's answer ...
}

public class Car
{
    private StateManager _manager { get; set; }

    private String color;

    public String Color
    {
        get { return color; }
        set {               
            string currentValue = color;
            _manager.ChangeState(new Command(() => color = value,
                                                    () => color = currentValue)); 
        }   
    }

    public Car()
    {
        _manager = new StateManager();
    }

    // Owners of Car instances can call car.StateHistory.RestorePreviousState() and/or
    // call car.StateHistory.RedoPreviousState()
    // but they can't easily the ChangeState method using the IStateHistory interface.
    public IStateHistory StateHistory { get { return StateManager; } }

}

An advantage of exposing the public IStateHistory StateHistory property of Car, instead of delegating via methods like public void Redo() { _manager.RedoPreviousState(); } is that there's less to edit if you want to make future changes like the ones which Sergey is suggesting:

Also I'd think about making public properties for checking available state changes. Because currently it's not clear whether state changed or not. You call UndoStateChange and nothing is returned whether state changed or there is nothing to undo.

You'd make these changes or additions to IStateHistory and StateManager, without needing to edit (add methods or properties to) the Car class.


Edit to reply to a new question/comment:

when I changed my Command class to match your example, the behavior change when the 'Action` e.g. this.Execute = action; was set. The event is being executed when Action property is set rather being executed when popped off the stack

I ran the following code (as a Console program):

using System;
using System.Collections.Generic;
using System.Linq;
using Debug = System.Console;

namespace Test
{
    public class Command
    {
        // public get and private set is similar to readonly
        public Action Execute { get; private set; }
        public Action Undo { get; private set; }
        public Command(Action action, Action undoAction)
        {
            Debug.WriteLine("Constructing Command");
            this.Execute = action;
            this.Undo = undoAction;
            Debug.WriteLine("Finished constructing Command");
        }
    }

    // this interface doesn't expose the ChangeState method
    public interface IStateHistory
    {
        void RestorePreviousState();
        void RedoPreviousState();
    }

    // The ChangeState method isn't in the IStateHistory interface,
    // and can be internal instead of public.
    public class StateManager : IStateHistory
    {
        private Stack<Command> _undoCommands = new Stack<Command>();
        private Stack<Command> _redoCommands = new Stack<Command>();
        public void ChangeState(Command command)
        {
            Debug.WriteLine("Invoking ChangeState");
            command.Execute();
            _undoCommands.Push(command);
            _redoCommands.Clear();
        }

        public void RestorePreviousState()
        {
            Debug.WriteLine("Invoking RestorePreviousState");
            if (!_undoCommands.Any())
                return;

            var command = _undoCommands.Pop();
            command.Undo();
            _redoCommands.Push(command);
        }

        public void RedoPreviousState()
        {
            Debug.WriteLine("Invoking RedoPreviousState");
            if (!_redoCommands.Any())
                return;

            var command = _redoCommands.Pop();
            command.Execute();
            _undoCommands.Push(command);
        }
    }

    public class Car
    {
        private StateManager _manager { get; set; }

        private String color;

        public String Color
        {
            get { return color; }
            set
            {
                string currentValue = color;
                _manager.ChangeState(new Command(
                    () => { color = value; Debug.WriteLine("New value is " + value); },
                    () => { color = currentValue; Debug.WriteLine("New value is " + currentValue); }));
            }
        }

        public Car()
        {
            _manager = new StateManager();
        }

        // Owners of Car instances can call car.StateHistory.RestorePreviousState() and/or
        // call car.StateHistory.RedoPreviousState()
        // but they can't easily the ChangeState method using the IStateHistory interface.
        public IStateHistory StateHistory { get { return _manager; } }

    }

    class Program
    {
        static void Main(string[] args)
        {
            Car car = new Car();
            car.Color = "Blue";
            car.Color = "Red";
            car.StateHistory.RestorePreviousState();
            car.StateHistory.RedoPreviousState();
        }
    }
}

It produces the following output:

Constructing Command
Finished constructing Command
Invoking ChangeState
New value is Blue
Constructing Command
Finished constructing Command
Invoking ChangeState
New value is Red
Invoking RestorePreviousState
New value is Blue
Invoking RedoPreviousState
New value is Red

Perhaps I didn't understand your latest question/comment, but IMO the output shows that the action is performed when the StateManager method is called, and not when this.Execute = action; was set inside the Command constructor.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ when I changed my Command class to match your example, the behavior change when the 'Action` e.g. this.Execute = action; was set. The event is being executed when Actionproperty is set rather being executed when popped off the stack \$\endgroup\$ – PhillyNJ Jan 18 '14 at 16:26
  • 1
    \$\begingroup\$ I edited my answer to try to reply to your comment. \$\endgroup\$ – ChrisW Jan 18 '14 at 18:08
  • \$\begingroup\$ you are correct. When I implemented this code into my bigger project, I introduced a bug, which I thought was caused by the Command. - Thank you \$\endgroup\$ – PhillyNJ Jan 19 '14 at 12: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.