views:

230

answers:

5

I'm trying to implement undo functionality with C# delegates. Basically, I've got an UndoStack that maintains a list of delegates implementing each undo operation. When the user chooses Edit:Undo, this stack pops off the first delegate and runs it. Each operation is responsible for pushing an appropriate undo delegate unto the stack. So suppose I have a method called "SetCashOnHand." It looks like this:

public void SetCashOnHand(int x) {
    int coh = this.cashOnHand;
    undo u = () => this.setCashOnHand(coh);
    this.cashOnHand = x;
    undoStack.Push(u);
}

So this method constructs an undo delegate, does the operation, and (assuming it succeeds) pushes the undo delegate onto the UndoStack. (UndoStack is smart enough that if undoStack.Push is called in the context of an undo, the delegate goes on the redo stack instead.)

My trouble is that it's a little annoying to "cache" this.cashOnHand into the coh variable. I wish I could just write this:

undo u = () => this.setCashOnHand(this.cashOnHand);

But of course that won't get the present value of cashOnHand; it defers looking up the value until the delegate is called, so the code winds up doing nothing. Is there any way I can "dereference" cashOnHand when constructing the delegate, other than stuffing the value into a local variable like coh?

I'm not really interested in hearing about better ways to do Undo. Please think of this as a generic question about delegates, with Undo just the example to make the question more clear.

+2  A: 

No, you'll have to capture the value of your instance variable outside of the lambda if you want it evaluated at that particular time.

It's no different than if you'd written this:

public void SetCashOnHand(int x) {
    this.cashOnHand = x;
    undoStack.Push(UndoSetCashOnHand);
}

private void UndoSetCashOnHand()
{
    this.setCashOnHand(this.cashOnHand);
}

The lambda syntax just make it slightly confusing since the evaluation appears to be a part of the declaring method body, when in reality it's part of an automatically-generated private function that gets evaluated just like any other function.

The way people generally get around this is by using a parameterized function and storing the value that gets passed to the function as part of the stack. If, however, you want to go with a parameter-less function, you'll have to capture it in a local.

Adam Robinson
I don't think this works.... when UndoSetCashOnHand is called, it will get the value of cashOnHand at undo-time, not at SetCOH time.
Jimmy
@Jimmy: That's exactly the point. This is what his lambda *does*, and he's asking if there's a way for it *not* to do that. The point I'm making is that it wouldn't make sense. The code wasn't intended to be an alternative solution, it's an alternative *representation* of what he's actually doing.
Adam Robinson
@Adam: I see. sorry for misunderstanding.
Jimmy
A: 

There isn't really any other magical way to do what you'd like. The reference is evaluated when the lambda is executed, not before. There isn't an automatic way to "hook" the value that's being closed over when the lambda was created other than what you're doing.

Greg D
+6  A: 

In general, there is no perfect solution to this other than what you're already doing.

However, in your specific case, you can make a currier, like this:

static Action Curry(Action<T> method, T param) { return () => method(param); }

You can use it like this:

Curry(setCashOnHand, this.cashOnHand);

If your method takes other parameters, you can use it like this:

Curry(cashOnHand => setCashOnHand(3, cashOnHand), this.cashOnHand);
SLaks
This seems like the best alternative. But it's so confusing, I think I'll just stick with copying the value to a local variable.
Paul A Jungwirth
Purity vs Maintenance. =)
Erik Forbes
A: 

The delegate is running in the future .. I think the temp variable is probably the best way to handle "run code now, instead of in the future." However, I guess you could write a Bind() function that binds a current value to a delegate..

Action Bind<T>(Func<T> f, T value) { 
    return (Action)(() => f(value));
}
undoStack.Push(Bind(this.setCashOnHand, this.cashOnHand));
Jimmy
oops, Curry is a better name :)
Jimmy
A: 

Well, if your Undo stack is referenced only by the instance that will be undoing its own actions, then it doesn't matter.

For instance:

public class MyType
{

  private UndoStack undoStack {get;set;}

  public void SetCashOnHand(int x) {
    int coh = this.cashOnHand;
    undo u = () => this.setCashOnHand(coh);
    this.cashOnHand = x;
    undoStack.Push(u);
   }
}

then it doesn't matter what the UndoStack references. If you're injecting an UndoStack and references to this instance are held elsewhere then it would matter.

If the second is true, I'd see about refactoring this rather than attempting to come up with some kind of weak reference or other chicanery to prevent instances being leaked via the undo stack.

I suppose in your case each level would have to have its own Undo stack. For instance, the UI layer would have to have an undo stack which would pop off the instance which needs to be undone next. Or, even, the groups of instances which need to undo, as one user click may require multiple instances of different types go through an undo procedure in order.

Will