My first thought is what is the valid lifetime of the reverse Action. If another Pop is done on the stack before you call reverse() what should happen?

Does each call to ReversiblePop give a different delegate that will only undo the pop it is associated with?

Regards,

Yes, you get a context sensitive delegate

I wasn't fond of the syntax at first, so I thought about something like this:

while(true)

{

actionStack.ConditionalPop(

action =>

  {

    if (action.ExecuteAt >= DateTime.Now)

        action.Execute();

  }

);

Thread.Sleep(1000);

}

But frankly I don't know if this is better. Hell, maybe it's worse? I figured an ActionSpecWrapper could intercept an Execute call and the ConditionalPop method could handle the Reverse() if no call was made to Execute().

I have only one comment:

'undo' would be a far better name than 'reverse'.

Other than that, I like this way to use undo, and I sometimes do it myself.

I'd also make a class

class UndoResult<T> {

public UndoResult(T value, Action undo) {

this.Value = value;

this.Undo = undo;

}

public T Value { get; set; }

public Action Undo { get; private set; }

}

and return that instead of an out parameter

Does this have any relation to transactions perhaps?

Can "Undo" be seen as a cross cutting concern?

Andrew mentioned transactions...what about a syntax akin to what you do with using (...) { tx.Commit() } ?

using (var action = actionStack.Pop())

using (var tx = actionStack.TransactionFor(action)) {

if (action.ExecuteAt >= DateTime.Now )

tx.Commit();

}

Why do you have Thread.Sleep(1000); in there?

Very nice but it could get complicated if you hold onto the reverse action too long, e.g. what happens if someone pops lots of items off the stack and then wants to reverse the first one? The semantics aren't clear to me.

Why not add a method PopFirst() to IActionStack, with PopFirst() accepting a delegate parameter that returns a bool.

E.g. delegate(ActionSpec action) { return action.ExecuteAt>=DateTime.Now; }

To me the intent is much clearer than a generic "reverse" - Reading the code two months later, what would you think it does? Why "reverse" it? Why not just not do something that needs to be reversed in the first place? Less chance of races.

Andrew,

Not in this scenario.

The stack is transactional, but I want to reverse something inside the transaction

Jim,

The sleep is here to demonstrate a full use case, nothing more.

I think that the part of the code with the abstraction you gave is in the correct place. There is the context so there is the decision.

However, I think you have wrong abstraction for both IActionStack interfaces you gave us. It is not really action stack it is just something like: IStack<ActionSpec> doesn't matter if it is transactional or not - that's why it's abstraction.

I think the better abstraction would be:

public interface IActionStack

{

ActionSpec PopAndExecute(); //can be void instead of ActionSpec

ActionSpec PopAndExecuteConditional(Predicate<ActionSpec> condition); //can be void instead of ActionSpec

void Push(ActionSpec action);

}

where Push is just incidentally there since I don't know specifics - but it could probably be more specific.

You could implement PopAndExecuteConditional something like:

var action = this.Pop();

if (condition(action))

    action.Execute();

else

    this.Push(action);

Or other thing you might do is to use something like TransactionScope ... UndoScope? And let your stack auto enroll into UndoScope operation :) So you might have something like:

using(UndoScope scope = new UndoScope(actionStack)){

   var action = actionStack.Pop();

if (action.ExecuteAt >= DateTime.Now ){

    action.Execute();

            scope.Complete();

   }

}

In scope.Complete() you would just mark that you don't need to rollback, and in dispose you would rollback if not mark not to rollback (yes, it gets a bit messy, and it's more of a hack :))

Cheers,

Mihailo

P.S. Oh, haven't seen Frank's post, it's similar.

Tell Don't Aak

while(true)

{

    var action = actionStack.ReversiblePop(DateTime.Now);

    action.Execute();


   Thread.Sleep(1000);;

}


public interface IActionStack

{

    ActionSpec Pop();

    ActionSpec ReversiblePop(DateTime validation);

    void Push(ActionSpec action);

}


public class ActionStack : IActionStack

{

    //..

    public ActionSpec ReversiblePop(DateTime validationDate)

    {

        var action = Pop();

        if (action.ExecuteAt(validationDate))

            return action;

        else

        {

            Push(action);

            return NullAction;

        }

    }   

}

Joseph,

Why should the stack know about its usages?

I think you all just rediscovered transactions.

public interface IActionStack

{

bool TryPop(Func<ActionSpec, bool> when, out ActionSpec value);

}

I was thinking of YAGNI. So I originally pushed the logic into the stack. The stack is nothing more than a data store for actions.

My new implementation is an 'Action Processor' with an added concern of the stack implementation. The client that utilizes the action does not care about the storage mechanism of the action. Now you can follow OCP. So the code could be this:

public interface IActionStack

{

    ActionSpec Pop();

    void Push(ActionSpec action);

}


public interface IActionProcessor

{

    void ReversibleExecute(DateTime onlyIfCorrectDateTime);

}


public class ActionProcessor : IActionProcessor

{

    private IActionStack actionStack; // DI

    //....


    public void ReversibleExecute(DateTime onlyIfCorrectDateTime)

    {

        var action = actionStack.Pop();

        if (action.ExecuteAt(onlyIfCorrectDateTime))

            action.Execute;

        else

            actionStack.Push(action);

    }  

}


public class ActionClient

{

    public void ExecuteAction()

    {

        while (true)

        {

            actionProcessor.ReversibleExecute(DateTime.Now);


            Thread.Sleep(1000);

        }

    }

}

What is cool about this is that you're using a blocking mechanism on processing actions with the ReversibleExecute method.

Do not process an Action until a certain time, also don't process other actions unitl the most recently added action is executed.

Is this not an implicit blocking scheme?

Why not implement a true explicit multi-threaded implementation for processing actions?