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'm looking for a design pattern or suggestions that can help refactor my code into something a bit less repetitive. I have a method that has several sequential steps (10-15) that if any of them fail must record detail about the failure to a log and rollback all previous portions of the transaction. The example is below.

I have thought about taking each try catch and turning it into an individual method but then I have to pass around my undo stack (among several other variables) and I still have to conditionally abort the method and process the undo stack. I've briefly looked into the Momento and Command patterns but both seemed to grow the line count by a fair margin.

EnumResult TransactionThatRollsBack()
{
        var undoLog = new Stack<Action>();
        try
        {
            MoveInventory(inv, src, dest);
            undoLog.Push(() => MoveInventory(inv,dest,src));
        }
        catch (Exception ex)
        {
            RecordError("InventoryError", ex, inv, src, dest);
            Undo(undoLog);
            return EnumResult.FailedInventoryMove;
        }

        try
        {
            NotifyReportingOfMove(inv, user);
            undoLog.Push(() => NotifyReportingOfUnMove(inv, user));
        }
        catch (Exception ex)
        {
            RecordError("NotifyReportingOfMove", ex, inv, user);
            Undo(undoLog);
            return EnumResult.FailedReportingMove;
        }

        try
        {
            AddUserToSweepstakes(user);
            undoLog.Push(() => RemoevUserFromSweepstakes(user));

        }
        catch (Exception ex)
        {
            RecordError("AddUserToSweepstakes", ex, user);
            Undo(undoLog);
            return EnumResult.FailedSweepstakesAdd;
        }

        ...
        return EnumResult.Success
}
share|improve this question

3 Answers 3

up vote 6 down vote accepted

You can remove repeated code if you record all your actions for executions and then execute them all in once with some generic exception handling. The idea is bellow.

Sorry for some possible lapses, didn't write in C# for quite some time. However I hope that you'll find the whole idea to be helpful.

  EnumResult TransactionThatRollsBack()
    {
         var undoLog = new SmartStack();
         undoLog.Push(() => MoveInventory(inv,dest,src), EnumResult.1);
         undoLog.Push(() => NotifyReportingOfUnMove(inv, user), EnumResult.2);
         undoLog.Push(() => RemoevUserFromSweepstakes(user), EnumResult.3);

         undoLog.execute();
    }

    class SmartStack {
       SortedDictionary<Action, EnumResult> actions; 

       void push(Action a, EnumResult r) {
         actions.add(a, r);
       }

       void execute() {
         Stack<Action> completedActions = new Stack<Action>();
         foreach(KeyValuePair<Action, EnumResult> pair in actions) {
             try {
               pair.Key.Invoke();
               completedActions.push(pair.Key);
             }
             catch(Exception ex) {
               for (Action a in completedActions) {
                   Undo(a);
               }
               return pair.Value;
             }
         }
       }
    }
share|improve this answer
    
Good idea, I had thought about the idea of (just like the undo log) using anonymous functions to represent the Perform, Undo, and Result on exception as well, looks like a possibly viable option. Seems to work good for single line calls, but if the content in the try is more than a line or 2 the nesting gets a bit ugly. –  deepee1 Feb 27 at 5:10
    
@AndreyTaptunov What would be in Undo(a) method? You can't invert arbitrary functions. –  abuzittin gillifirca Mar 3 at 12:29

Well I would personally want to cut down on all of that exception handling code.

The easiest way in my mind is to create your own exceptions. Now, I understand you seem to need a message and enum associated with each chunk there so why not make that as a base transactionException and overload each case.

public class TransactionException : Exception
{
    public TransactionException(string message,EnumResult result,Exception innerException) : base(message,innerException) { Result = result; }

    public EnumResult Result { get; private set; }
}

Encapsulates the basic functionality of a transaction and now in specific cases you can do something like:

public class InventoryMoveException : TransactionException
{
    public InventoryMoveException(Exception innerException) : this("InventoryError",innerException) {}
    public InventoryMoveException(string message,Exception innerException) : base(message,EnumResult.FailedInventoryMove,innerException){}
}

and with all that in place you should be able to throw a number of custom exceptions and wrap the whole thing in one catch block that will log the associated message and return the associated enum.

share|improve this answer
    
I like this approach. It provides relatively readable code as well in the method i'm talking about. Only downside is that I'd have to catch the exceptions in all of the calling methods and wrap them in a new Transaction exception, some of which are not my code base, so I'd have to make wrapper methods for those. Thanks for the idea. –  deepee1 Feb 27 at 14:37
    
@deepee1 Good! That is exactly the point. You SHOULD have a nested exception chain and can react at the desired point Transaction Failed-> TransactionException->UserAdditionException->NullArgumentException . you can fail graciously and still give an informative stack trace. bubble the exception. In fact I am against the idea of Error Enums in the first place, that swallows the exception. in your example you will know a userException occured but not on what object or under what circumstance. keep throwing exceptions till Presentation layer,let the final use circumstance dictate error handling. –  apieceoffruit Feb 27 at 14:47
    
you could add probably add Undo(undoLog); to the exceptions as well –  Malachi Feb 27 at 14:57
1  
@Malachi well I'd be hesitant of coupling logging with exceptions, I only added the message because the exception has a message anyway, they are all in the same domain. I would be more inclined to fire off a log event and add undoLog into the EventArgs, allowing any kind of logging subscriptions. (although tbh it would actually be a composite logger interface but you get the idea..) –  apieceoffruit Feb 27 at 15:22
    
that sounds about right. –  Malachi Feb 27 at 17:42

Much of that needs to be there. You could modify things to use a single try-catch though:

    var undoLog = new Stack<Action>();
    var errorContainer = new ErrorContainer{ message = "Unknown error", enumResult = EnumResult.UnknownError };

    try
    {
        errorContainer = GenerateError(EnumResult.FailedInventoryMove, "InventoryError", inv, src, dest);
        MoveInventory(inv, src, dest);
        undoLog.Push(() => MoveInventory(inv,dest,src));

        errorContainer = GenerateError(EnumResult.FailedReportingMove, "NotifyReportingOfMove", inv, user);
        NotifyReportingOfMove(inv, user);
        undoLog.Push(() => NotifyReportingOfUnMove(inv, user));

        ...

        return EnumResult.Success;
    }
    catch (Exception ex)
    {
        Undo(undoLog);
        LogError(errorContainer, ex);
        return errorContainer.enumResult;
    }

I don't know that you gain clarity by going this route, but if you can make the ErrorContainer work for you, it might be an option to reduce total lines of code.

share|improve this answer
    
Thanks for the feedback. 1 hiccup on a single catch... Each part of the transaction that fails has to return a different Enum result and they may share similar exceptions. –  deepee1 Feb 27 at 5:05
    
I knew I forgot something. The EnumResult should have been added as an argument to GenerateError. Updated answer. –  Rob Ribeiro Feb 27 at 5:43

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.