I got a lot of responses for my refactoring challenge. My answer for that is here. Broadly, I performed a method to a method object refactoring. If you look at the git history, you can see each step as an independent commit.

public class MessageHandlingCompletion
{
    private readonly TransactionScope tx;
    private readonly Action sendMessageBackToQueue;
    private readonly Action<CurrentMessageInformation, Exception> messageCompleted;
    private readonly Action<CurrentMessageInformation> beforeTransactionCommit;
    private readonly ILog logger;
    private readonly Action<CurrentMessageInformation, Exception> messageProcessingFailure;
    private readonly CurrentMessageInformation currentMessageInformation;

    private Exception exception;

    public MessageHandlingCompletion(TransactionScope tx, 
        Action sendMessageBackToQueue, Exception exception, 
        Action<CurrentMessageInformation, Exception> messageCompleted, 
        Action<CurrentMessageInformation> beforeTransactionCommit, 
        ILog logger, Action<CurrentMessageInformation, Exception> messageProcessingFailure, 
        CurrentMessageInformation currentMessageInformation)
    {
        this.tx = tx;
        this.sendMessageBackToQueue = sendMessageBackToQueue;
        this.exception = exception;
        this.messageCompleted = messageCompleted;
        this.beforeTransactionCommit = beforeTransactionCommit;
        this.logger = logger;
        this.messageProcessingFailure = messageProcessingFailure;
        this.currentMessageInformation = currentMessageInformation;
    }


    public void HandleMessageCompletion()
    {
        var txDisposed = false;

        try
        {
            if (SuccessfulCompletion(out txDisposed))
                return;
        }
        finally
        {
            DisposeTransactionIfNotAlreadyDisposed(txDisposed);                
        }

        //error

        NotifyMessageCompleted();

        NotifyAboutMessageProcessingFailure();

        SendMessageBackToQueue();
    }

    private void SendMessageBackToQueue()
    {
        if (sendMessageBackToQueue != null)
            sendMessageBackToQueue();
    }

    private void NotifyMessageCompleted()
    {
        try
        {
            if (messageCompleted != null)
                messageCompleted(currentMessageInformation, exception);
        }
        catch (Exception e)
        {
            logger.Error("An error occured when raising the MessageCompleted event, the error will NOT affect the message processing", e);
        }
    }

    private void NotifyAboutMessageProcessingFailure()
    {
        try
        {
            if (messageProcessingFailure != null)
                messageProcessingFailure(currentMessageInformation, exception);
        }
        catch (Exception moduleException)
        {
            logger.Error("Module failed to process message failure: " + exception.Message,
                         moduleException);
        }
    }

    private void DisposeTransactionIfNotAlreadyDisposed(bool txDisposed)
    {
        try
        {
            if (txDisposed == false && tx != null)
            {
                logger.Warn("Disposing transaction in error mode");
                tx.Dispose();
            }
        }
        catch (Exception e)
        {
            logger.Warn("Failed to dispose of transaction in error mode.", e);
        }
    }

    private bool SuccessfulCompletion(out bool txDisposed)
    {
        txDisposed = false;
        if (exception != null)
            return false;
        try
        {
            if (tx != null)
            {
                if (beforeTransactionCommit != null)
                    beforeTransactionCommit(currentMessageInformation);
                tx.Complete();
                tx.Dispose();
                txDisposed = true;
            }
            NotifyMessageCompleted();
            return true;
        }
        catch (Exception e)
        {
            logger.Warn("Failed to complete transaction, moving to error mode", e);
            exception = e;
        }
        return false;
    }
}

The code can still be improved, I think, but it stopped being an eye sore, and along the way I managed to reduce duplication and simplify future work, I call it a win.

One of the more interesting results from the replies that I got is that a lot of people didn’t actually read the code.

The major reason that the code was complex was that there were several concerns that had to be dealt with, but the reason that this method dealt with several concerns at the same time was that different actions are required for different errors, that the error handling code itself need to be aware of errors, etc.

One of the nicer suggestions that I got was to create a strategy chain, like this:

public class TransactionBehavior : BaseBehavior
{
    public void Execute()
    {
        using(var tx = new TransactionScope())
        {
            NextBehavior.Execute();
            tx.Complete();
        }
    }
}

public class ExceptionBehavior : BaseBehavior
{
    public void Execute()
    {
        try
        {
            NextBehavior.Execute();
        }
        catch(Exception ex)
        {
            logger.Error(ex.Message, ex);
        }
    }
}

public class ConsumeBehavior : BaseBehavior
{
    public void Execute()
    {
        foreach(var consumer in GatherConsumers())
        {
            reflection.InvokeConsume(consumer);
        }
        NextBehavior.Execute();
    }
}

And that looks really nice, but it suffer from one major problem, it doesn’t (and can’t) handle error scenarios in a good way when different errors have different meanings and requires different responses.

I got some comments that were very critical about the code, but it is quite telling that about 90% of the actual refactoring attempts ended up dropping some critical functionality.