Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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>  where T : IParameter
    {
        bool GetParameterFromParentOperation { get; }

        OperationStatus Status { get; set; }

        IParameter Execute(IParameter 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(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

  • What you guys think about it;
  • 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 other pattern?
  • Do you see any possible future headache on this, or maybe 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.


EDIT

Code edited due to @Dan Lyons suggestion removed unnecessary IExecutableOperation<T> and IInternalOperation<T>, joined their signatures to IOperation<out T>.


EDIT2

Per @svick great suggestions, the code now looks like this. Now I don't even remember why I was so lost with covariance issues, this didn't even needed covariance usage.

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
    {
        public bool InternalDebitDone { get; set; }

        public bool InterbankCreditDone { get; set; }

        public bool CommissionDone { get; set; }
    }

    public class InternalDebitOperation : Operation<InterbankTranferenceInfo>, IReversible
    {
        public InternalDebitOperation(InterbankTranferenceInfo parameter)
            : base(parameter)
        {
        }

        protected override InterbankTranferenceInfo ExecuteInternal()
        {
            this.Parameter.InternalDebitDone = true;
            return base.ExecuteInternal();
        }

        public void Reverse()
        {
            this.Parameter.InterbankCreditDone = false;
        }
    }

    public class InterbankCreditOperation : Operation<InterbankTranferenceInfo>, IReversible
    {
        protected override InterbankTranferenceInfo ExecuteInternal()
        {
            this.Parameter.InterbankCreditDone = true;

            return base.ExecuteInternal();
        }

        public void Reverse()
        {
            this.Parameter.InterbankCreditDone = false;
        }
    }

    public class CommissionOperation : Operation<InterbankTranferenceInfo>, IReversible
    {
        protected override InterbankTranferenceInfo ExecuteInternal()
        {
            // Uncomment this code to test Reverse operation.
            // throw new Exception("Test exception, it should trigger Reverse() method.");

            this.Parameter.CommissionDone = true;

            return base.ExecuteInternal();
        }

        public void Reverse()
        {
            this.Parameter.CommissionDone = false;
        }
    }

    public interface IReversible
    {
        void Reverse();
    }

    public enum OperationStatus
    {
        Done,
        Pending,
        Reversed
    }

    public interface IOperation<T>
    {
        OperationStatus Status { get; set; }

        T Parameter { get; set; }

        T Execute(T parameter);
    }

    public abstract class Operation<T> : IOperation<T>
    {
        public T Parameter { get; set; }

        public OperationStatus Status { get; set; }

        protected Operation()
        {
            Status = OperationStatus.Pending;
        }

        protected Operation(T parameter)
        {
            Parameter = parameter;
        }

        protected virtual T ExecuteInternal()
        {
            return this.Parameter;
        }

        T IOperation<T>.Execute(T parameter)
        {
            Parameter = parameter;
            return ExecuteInternal();
        }
    }

    public class Orchestrator<T>
    {
        public IOperation<T>[] Operations { get; private set; }

        public Orchestrator(params IOperation<T>[] operations)
        {
            Operations = operations;
        }

        public T Run()
        {
            T previousOperationResult = default(T);

            foreach (var operation in this.Operations)
            {
                try
                {
                    if (previousOperationResult == null)
                        previousOperationResult = operation.Execute(operation.Parameter);
                    else
                        previousOperationResult = operation.Execute(previousOperationResult);

                    operation.Status = OperationStatus.Done;
                }
                catch (Exception ex)
                {
                    foreach (var o in this.Operations)
                    {
                        if (o is IReversible)
                        {
                            if (o.Status == OperationStatus.Done)
                            {
                                ((IReversible)o).Reverse();
                                o.Status = OperationStatus.Reversed;
                            }
                        }
                    }

                    throw new ReversedException(string.Format("There's been a problem on operation {0}. All the reversable operations has been reversed.", operation.GetType().Name), ex);
                }
            }

            return previousOperationResult;
        }
    }


    public class ReversedException : Exception
    {
        public ReversedException(string message, Exception ex)
            : base(message, ex)
        {
        }
    }
share|improve this question
add comment

2 Answers 2

up vote 2 down vote accepted

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).
share|improve this answer
    
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 Apr 24 '13 at 20:54
    
Nice! I'll work a little on it, I'd like to remove that "Initial Operation" concept, will post back soon! –  Daniel Apr 24 '13 at 20:59
    
Check the updated answer, I did a little modification in order to merge IInitialOperation and IOperation. –  Daniel Apr 24 '13 at 21:39
    
@Daniel Going from first to last is exactly the problem, I think it makes the most sense to go from last executed to first. –  svick Apr 24 '13 at 22:28
    
Thanks, but in this case I have to undo the path in the order it was executed. –  Daniel Apr 25 '13 at 4:11
add comment

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;
      }
   }
share|improve this answer
    
Thanks, but your code is not compiling. You can't do this: T Execute(T parameter);, setting the generic as a parameter type. –  Daniel Apr 24 '13 at 18:13
    
On the other hand, IExecutableOperation<T> or IInternalOperation<T> will be removed, thanks for noticing, their signature can be directly on IOperation. –  Daniel Apr 24 '13 at 18:25
    
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 Apr 24 '13 at 18:38
    
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. –  Daniel Apr 24 '13 at 18:46
    
@Daniel How is object any worse than IParameter? And I think the amount of casting would be the same. –  svick Apr 24 '13 at 19:28
show 1 more comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.