Undo/Redo Implementation For Multiple Variables

Question

I'm trying to refactor an undo/redo implementation I have but am unsure how to go about it.

public class MyObject
{
    public int A;
    public int B;
    public int C;
}

public abstract class UndoRedoAction
{
    protected MyObject myobj;
    protected int oldValue;
    protected int newValue;

    public abstract void Undo();
    public abstract void Redo();
}

public class UndoRedoActionA : UndoRedoAction
{
    UndoRedoActionA(MyObject obj, int new)
    {
        myobj = obj;
        oldValue = myobj.A;
        newValue = new;
        myobj.A = newValue;
    }

    public override void Undo()
    {
        myobj.A = oldValue;
    }

    public override void Redo()
    {
        myobj.A = newValue;
    }    
}

public class UndoRedoActionB : UndoRedoAction
{
    UndoRedoActionB(MyObject obj, int new)
    {
        myobj = obj;
        oldValue = myobj.B;
        newValue = new;
        myobj.B = newValue;
    }

    public override void Undo()
    {
        myobj.B = oldValue;
    }

    public override void Redo()
    {
        myobj.B = newValue;
    }    
}

public class UndoRedoActionC : UndoRedoAction
{
    UndoRedoActionC(MyObject obj, int new)
    {
        myobj = obj;
        oldValue = myobj.C;
        newValue = new;
        myobj.C = newValue;
    }

    public override void Undo()
    {
        myobj.C = oldValue;
    }

    public override void Redo()
    {
        myobj.C = newValue;
    }    
}

Obviously, each of the UndoRedoAction child classes have custom functionality as they access different fields, but the functionality they perform on those fields are identical. Is there any clean way, short of making those ints into properties and passing property names (which I would rather not do, magic strings and such), to combine these into a generic UndoRedoAction instead of making a bunch of child classes that all perform the exact same actions on different variables?

I did consider using the Memento pattern which would solve the problem, but it seems quite overkill for a scenario this small and I have no one-way actions to worry about, which is where the Memento pattern is really useful.

Thanks.

Clarification: These UndoRedoAction objects are placed inside a Stack<UndoRedoAction> which serves as the undo cache. More specifically, there are two stacks, one for undo, one for redo, and actions popped off one are pushed onto the other. Additionally, in response to Zaid Masud's response, the variables are not necessarily all ints, or even all the same object type. My example merely did that for simplicity.

Answer 1

You can avoid reflection by using dynamic methods or expressions to create a custom property getter/setter based on a selector expression like x => x.SomeProperty.

Basically you'd want to do something like this:

public class PropertySetActionProvider<TObj, TProp>
{
    private Func<TObj, TProp> _getter;
    private Action<Tbj, TProp> _setter;

    public PropertySetActionProvider(Expression<Func<TObj, TProp>> propertySelector)
    {
        _getter = propertySelector.Compile();
        _setter = SetterExpressionFromGetterExpression(propertySelector).Compile(); 
    }

    public IUndoRedoAction CreateAction(TObj target, TProp newValue)
    {
        var oldValue = _getter(target);
        return new PropertySetAction<TObj, TProp>(_setter, target, oldValue, newValue);             
    }
}

public class PropertySetAction<TObj, TProp> : IUndoRedoAction 
{
   private Action<TObj, TProp> _setter;
   private TObj _target;
   private TProp _oldValue;
   private TProp _newValue;

   public PropertySetAction(Action<TObj, TProp> setter, TObj target, TProp oldValue, TProp newValue)
   {
        _setter = setter; 
        _target = target; 
        _oldValue = oldValue; 
        _newValue = newValue;
   }

   public void Do() 
   {
       _setter(_target, _newValue);
   }  

   public void Undo() 
   {
       _setter(_target, _oldValue);
   }   
}

Then you can easily create new actions with code like this:

  // create the action providers once per property you'd like to change
  var actionProviderA = new PropertySetActionProvider<MyObject, int>(x => x.A);
  var actionProviderB = new PropertySetActionProvider<MyObject, string>(x => x.B);

  var someObject = new MyObject { A = 42, B = "spam" };
  actions.Push(actionProviderA.CreateAction(someObject, 43);
  actions.Push(actionProviderB.CreateAction(someObject, "eggs");
  actions.Push(actionProviderA.CreateAction(someObject, 44);
  actions.Push(actionProviderB.CreateAction(someObject, "sausage");

  // you get the point

The only hard part is creating a setter expression from the getter expression (the SetterExpressionFromGetterExpression method above), but that's a known solved problem. See for example here and here for questions about how to do so.

In this approach the cost of compiling the getter/setter delegates from the expression is incurred only once when you create the action provider, not every time you create an action or invoke Redo or Undo on one.

If you want to further optimize, you can move the propertySelector parameter inside the action constructor, and have the action providers be created behind the scenes on demand and cached on a per-property basis. This will yield somewhat easier to use code but might be trickier to implement.

Hope this helps!

Answer 2

For that kind of problem I prefer using a Stack<T> or List<T> and copy my whole object inside it. This allows having more Undo cache than one and it is straightforward. I used this model to allow user 10 undo for a complicated object. By the way this requires a serializable class usage. Here is my code to make an exact copy of my classes:

        private T DeepCopy<T>(T obj)
    {
        object result = null;
        if (obj == null)
        {
            return (T)result;
        }
        using (var ms = new MemoryStream())
        {
            var formatter = new BinaryFormatter();
            formatter.Serialize(ms, obj);
            ms.Position = 0;

            result = (T)formatter.Deserialize(ms);
            ms.Close();
        }

        return (T)result;
    }

EDIT: for a simple usage with List

        List<MyClass> UndoCache = new List<MyClass>();
    MyClass myRealObject;
    int unDoCount = 10;
       void Undo()
    {
        myRealObject = UndoCache.Last();
        //remove this object from cache.
        UndoCache.RemoveAt(UndoCache.Count - 1);
    }

//call this method when your object is changed

        void ObjectChanged()
    {
        //remove the first item if we reach limit
        if (UndoCache.Count > unDoCount)
        {
            UndoCache.RemoveAt(0);
        }
        UndoCache.Add(DeepCopy<MyClass>(myRealObject));
    }
    public class MyClass { }

Answer 3

I suppose if all the fields in MyObject are int as in your code sample, you can convert them into an array of fixed length. Code sample (not compiled)

public class MyObject
{
    public const int ARRAY_LENGTH = 3;
    public int[] Ints = new int[ARRAY_LENGTH]
}

public abstract class UndoRedoAction
{
    private MyObject myobj;
    private int oldValues = new int[MyObject.ARRAY_LENGTH];
    private int newValues = new int[MyObject.ARRAY_LENGTH];

    public UndoRedoAction(MyObject obj)
    {
        myobj = obj;
    }

    public void SetValue(int index, int newValue)
    {
        oldValues[index] = myObj.Ints[index];
        newValues[index] = newValue;
        myObj.Ints[index] = newValue;
    }

    public void Undo(int index)
    {
        myObj.Ints[index] = oldValues[index];
    }

    public void Redo(int index)
    {
        myObj.Ints[index] = newValues[index];
    }
}