patterncsharpMinor
follow-up review of a custom Undo Manager
Viewed 0 times
customfollowmanagerreviewundo
Problem
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 (
```
public interface ICommand
{
// Executes this command.
void Execute();
// Undoes this command.
void Undo();
}
///
/// The Command class holds a reference to an Event and the Undo Event
///
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 _undoCommands = new Stack();
private Stack _redoCommands = new Stack();
public void ChangeState(Command command)
{
command.Execute();
_undoCommands.Push(command);
}
public void RestorePreviousState()
{
if (this._undoCommands.Any()) {
var command = _undoCommands.Pop();
command.Undo();
_redoCommands.Cl
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. 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();
}
///
/// The Command class holds a reference to an Event and the Undo Event
///
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 _undoCommands = new Stack();
private Stack _redoCommands = new Stack();
public void ChangeState(Command command)
{
command.Execute();
_undoCommands.Push(command);
}
public void RestorePreviousState()
{
if (this._undoCommands.Any()) {
var command = _undoCommands.Pop();
command.Undo();
_redoCommands.Cl
Solution
I think you should not clear
Also I'm thinking on improving naming... because
UPDATE: I'd go with
_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.Code Snippets
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);
}Context
StackExchange Code Review Q#39503, answer score: 3
Revisions (0)
No revisions yet.