Sequential Execution: Orchestrator Pattern

Question

I built a code piece that makes it easy to execute a set of sequential operations, that might depend on the same parameter value (passed across the execution) or might need a particular parameter. There's also a possibility to manage an error from within the operations, by implementing IReversible interface.

This code example can be copy/pasted into a console app, so you can easily check the functionality.

So far, this is what I've done:

 class Program
 {
 static void Main(string[] args)
 {
 var transferenceInfo = new InterbankTranferenceInfo();
 var orchestrator = new Orchestrator(new InternalDebitOperation(transferenceInfo),
 new InterbankCreditOperation(),
 new CommissionOperation());
 orchestrator.Run();
 }
 }
 public class InterbankTranferenceInfo : IParameter
 {
 public bool InternalDebitDone { get; set; }
 public bool InterbankCreditDone { get; set; }
 public bool CommissionDone { get; set; }
 }
 public class InternalDebitOperation : Operation<InterbankTranferenceInfo>, IOperation<InterbankTranferenceInfo>
 {
 public InternalDebitOperation(InterbankTranferenceInfo parameter)
 : base(parameter)
 {
 }
 public override InterbankTranferenceInfo Execute()
 {
 return new InterbankTranferenceInfo() { InternalDebitDone = true };
 }
 }
 public class InterbankCreditOperation : Operation<InterbankTranferenceInfo>, IOperation<InterbankTranferenceInfo>
 {
 public override InterbankTranferenceInfo Execute()
 {
 Parameter.InterbankCreditDone = true;
 return Parameter;
 }
 }
 public class CommissionOperation : Operation<InterbankTranferenceInfo>, IReversible, IOperation<InterbankTranferenceInfo>
 {
 public override InterbankTranferenceInfo Execute()
 {
 Parameter.CommissionDone = true;
 // Uncomment this code to test Reverse operation.
 // throw new Exception("Test exception, it should trigger Reverse() method.");
 return Parameter;
 }
 public void Reverse()
 {
 Parameter.CommissionDone = false;
 }
 }
 public enum OperationStatus
 {
 Done,
 Pending,
 Reversed
 }
 public interface IParameter
 {
 }
 public interface IReversible
 {
 void Reverse();
 }
 public interface IOperation<out T> : IInternalOperation<T> where T : IParameter
 {
 }
 public interface IInternalOperation<out T> : IExecutableOperation<T>
 {
 bool GetParameterFromParentOperation { get; }
 OperationStatus Status { get; set; }
 IParameter Execute(IParameter parameter); 
 }
 public interface IExecutableOperation<out T>
 {
 T Execute();
 }
 //[System.Diagnostics.DebuggerStepThroughAttribute()]
 public abstract class Operation<T> : IInternalOperation<T> where T : IParameter
 {
 public T Parameter { get; private set; }
 public bool GetParameterFromParentOperation { get { return this.Parameter == null; } }
 public OperationStatus Status { get; set; }
 public Operation()
 {
 Status = OperationStatus.Pending;
 }
 public Operation(IParameter parameter)
 {
 Status = OperationStatus.Pending;
 this.Parameter = (T)parameter;
 }
 public abstract T Execute();
 public virtual IParameter Execute(IParameter parameter)
 {
 this.Parameter = (T)parameter;
 return this.Execute();
 }
 }
 public class Orchestrator
 {
 public List<IOperation<IParameter>> Operations { get; private set; }
 public Orchestrator(params IOperation<IParameter>[] operations) 
 {
 this.Operations = new List<IOperation<IParameter>>();
 foreach (var item in operations)
 {
 this.Operations.Add((IOperation<IParameter>)item);
 }
 }
 public IParameter Run()
 {
 IParameter previousOperationResult = null;
 foreach (var operation in this.Operations)
 {
 try
 {
 if (operation.GetParameterFromParentOperation)
 previousOperationResult = operation.Execute(previousOperationResult);
 else
 previousOperationResult = operation.Execute();
 operation.Status = OperationStatus.Done;
 }
 catch (Exception)
 {
 foreach (var o in this.Operations)
 {
 if (o is IReversible)
 {
 ((IReversible)o).Reverse();
 o.Status = OperationStatus.Reversed;
 }
 else
 throw;
 }
 break;
 }
 }
 return previousOperationResult;
 }
 }

Questions:

1. What you think about it?
2. Am I reinventing the wheel? Is there any pattern or fwk that covers this functionality? It looks like command pattern with steroids. Or is it another pattern?
3. Do you see any possible future headache on this or have any suggestion about how to improve it?

I don't like the implementation of both Operation and IOperation on concrete types, so feel free to correct me if there's a more elegant way of doing this covariant scenario.

Answer 1

First, some structural issues:

1. I think the way you're using generics is completely wrong. The generic parameter T is almost useless IInternalOperation<T> (since you're using IParameter there, not T). And if a class has a method IParameter Execute(IParameter parameter), it means that method should accept any IParameter, which is not what you want.

2. Orchestrator works with IParameter, even if all the operations work with some specific subtype. I think Orchestrator should be generic too.

3. Some of your operations don't work correctly if they are called with one overload of Execute(), the rest don't work correctly when called with the other overload. I think that you should have two different interfaces: one for the initial operation (with the parameterless overload of Execute()) and one for the rest of the operations (with the other overload). And Orchestrator should enforce this by taking a single initial operation and a collection of following operations.

4. I think the marker interface IParameter is useless.

With these modifications, your code could look something like:

public interface IOperationBase
{
 OperationStatus Status { get; set; }
}
public interface IOperation<T> : IOperationBase
{
 void Execute(T parameter);
}
public interface IInitialOperation<T> : IOperationBase
{
 T Execute();
}
public abstract class OperationBase<T> : IOperationBase
{
 public T Parameter { get; protected set; }
 public OperationStatus Status { get; set; }
 protected OperationBase()
 {
 Status = OperationStatus.Pending;
 }
}
public abstract class Operation<T> : OperationBase<T>, IOperation<T>
{
 public void Execute(T parameter)
 {
 Parameter = parameter;
 ExecuteInternal();
 }
 protected abstract void ExecuteInternal();
}
public abstract class InitialOperation<T> : OperationBase<T>, IInitialOperation<T>
{
 protected InitialOperation(T parameter)
 {
 Parameter = parameter;
 }
 public abstract T Execute();
}
public class Orchestrator<T>
{
 public IInitialOperation<T> InitialOperation { get; private set; }
 public IOperation<T>[] Operations { get; private set; }
 public Orchestrator(
 IInitialOperation<T> initialOperation, params IOperation<T>[] operations)
 {
 InitialOperation = initialOperation;
 Operations = operations;
 }
 public T Run()
 {
 // omitted
 }
}

One more thing regarding your implementation: the way you roll back the operations on exception doesn't make much sense to me. When rolling back, you usually:

• Roll back only the operations that actually succeeded (your code will try to roll back even operations that didn't actually run).
• Roll back the operations in reverse order: the last operation that ran first, the first operation last.
• Indicate to the caller that the whole operation failed. Either by rethrowing the exception after the rollback completes, or at least by returning false (and true on success).

• Daniel: About the roll backs, the only issue is to check if the operation was completed, and erroring out instead of the break; statement. I think the code already goes from the first to last operation to reverse. Thanks for the advice!
• Daniel: Nice! I'll work a little on it, I'd like to remove that "Initial Operation" concept, will post back soon!
• Daniel: Check the updated answer, I did a little modification in order to merge IInitialOperation and IOperation.
• svick: @Daniel Going from first to last is exactly the problem, I think it makes the most sense to go from last executed to first.
• Daniel: Thanks, but in this case I have to undo the path in the order it was executed.

Answer 2

It does not look like you ever reference IExecutableOperation<T> or IInternalOperation<T> directly, so I would take the YAGNI approach and roll them both into IOperation<T>.

With that done, Operation<T> would implement IOperation<T> directly, so you no longer have to explicitly mark InternalDebitOperation, InterbankCreditOperation, and CommissionOperation as implementing IOperation<T>.

Additional Comments/Concerns:

• There is the potential for InvalidCastExceptions when calling your IParameter Execute (IParameter parameter) overload, because you cast an IParameter to type T, which is a sub-type of IParameter, but there is nothing preventing you from passing a different IParameter implementation in which cannot be cast to T.
• See previous comment about your Operation<T> constructor.
• The way in which Orchestrator is coded forces you to do everything in the context of IParameter. This somewhat defeats the purpose of making Operation<T> generic, because you're having to box/unbox your parameter types between T and IParameter anyways. I would suggest either making Orchestrator generic as well. The alternative is making IOperation/Operation non-generic.

With those comments in mind, I would make the following changes:

 class Program
 {
 static void Main (string [] args)
 {
 var transferenceInfo = new InterbankTranferenceInfo ();
 var orchestrator = new Orchestrator<InterbankTranferenceInfo> (new InternalDebitOperation (transferenceInfo),
 new InterbankCreditOperation (),
 new CommissionOperation ());
 orchestrator.Run ();
 }
 }
 public class InterbankTranferenceInfo : IParameter
 {
 public bool InternalDebitDone { get; set; }
 public bool InterbankCreditDone { get; set; }
 public bool CommissionDone { get; set; }
 }
 public class InternalDebitOperation : Operation<InterbankTranferenceInfo>
 {
 public InternalDebitOperation (InterbankTranferenceInfo parameter)
 : base (parameter)
 {
 }
 public override InterbankTranferenceInfo Execute ()
 {
 return new InterbankTranferenceInfo () { InternalDebitDone = true };
 }
 }
 public class InterbankCreditOperation : Operation<InterbankTranferenceInfo>
 {
 public override InterbankTranferenceInfo Execute ()
 {
 Parameter.InterbankCreditDone = true;
 return Parameter;
 }
 }
 public class CommissionOperation : Operation<InterbankTranferenceInfo>, IReversible
 {
 public override InterbankTranferenceInfo Execute ()
 {
 Parameter.CommissionDone = true;
 // Uncomment this code to test Reverse operation.
 // throw new Exception("Test exception, it should trigger Reverse() method.");
 return Parameter;
 }
 public void Reverse ()
 {
 Parameter.CommissionDone = false;
 }
 }
 public enum OperationStatus
 {
 Done,
 Pending,
 Reversed
 }
 public interface IParameter
 {
 }
 public interface IReversible
 {
 void Reverse ();
 }
 public interface IOperation<out T> where T : IParameter
 {
 bool GetParameterFromParentOperation { get; }
 OperationStatus Status { get; set; }
 T Execute (T parameter);
 T Execute ();
 }
 //[System.Diagnostics.DebuggerStepThroughAttribute()]
 public abstract class Operation<T> : IOperation<T> where T : IParameter
 {
 public T Parameter { get; private set; }
 public bool GetParameterFromParentOperation { get { return this.Parameter == null; } }
 public OperationStatus Status { get; set; }
 public Operation ()
 {
 Status = OperationStatus.Pending;
 }
 public Operation (T parameter)
 : this ()
 {
 this.Parameter = parameter;
 }
 public abstract T Execute ();
 public virtual T Execute (T parameter)
 {
 this.Parameter = parameter;
 return this.Execute ();
 }
 }
 public class Orchestrator<T> where T:IParameter
 {
 public List<IOperation<T>> Operations { get; private set; }
 public Orchestrator (params IOperation<T> [] operations)
 {
 this.Operations = new List<IOperation<T>> ();
 foreach (var item in operations)
 {
 this.Operations.Add (item);
 }
 }
 public T Run ()
 {
 T previousOperationResult = default (T);
 foreach (var operation in this.Operations)
 {
 try
 {
 if (operation.GetParameterFromParentOperation)
 previousOperationResult = operation.Execute (previousOperationResult);
 else 
 previousOperationResult = operation.Execute ();
 operation.Status = OperationStatus.Done;
 }
 catch (Exception)
 {
 foreach (var o in this.Operations)
 {
 if (o is IReversible)
 {
 ((IReversible) o).Reverse ();
 o.Status = OperationStatus.Reversed;
 }
 else
 throw;
 }
 break;
 }
 }
 return previousOperationResult;
 }
 }

• Daniel: Thanks, but your code is not compiling. You can't do this: T Execute(T parameter);, setting the generic as a parameter type.
• Daniel: On the other hand, IExecutableOperation<T> or IInternalOperation<T> will be removed, thanks for noticing, their signature can be directly on IOperation.
• Daniel: Please explain me how can occur an InvalidCastExceptions, I don't see how can it happen, there's always a constraint clause to T to be a IParameter.
• Daniel: About: "Orchestrator is coded forces you to do everything in the context of IParameter..." It's that complex because of covariance, if you find any other way of doing it, let me know! Making non-generics operation would lead to a sort of anarchy when passing parameters between Operations, would be just object types and leave too much casting responsibility to concrete types.
• svick: @Daniel How is object any worse than IParameter? And I think the amount of casting would be the same.