Ayende @ Rahien

Refunds available at head office

Refactor This: Results

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.

Comments

[ICR]
04/06/2010 11:43 AM by
[ICR]

Why not put txDisposed as a member of the class? That would avoid the ugly out parameter.

Jeff Sternal
04/06/2010 01:27 PM by
Jeff Sternal

Isn't there some way to make the creator of the TransactionScope responsible for disposing it? It's hard to see why that should be a concern of this method or class.

Ken Egozi
04/06/2010 02:19 PM by
Ken Egozi

nice.

I'd replace the SuccessfulCompletion method to return a struct of two booleans, IsSuccessful and WasDisposed

@ICR: Better than having an unneeded member, and nicer than the out param

NPehrsson
04/06/2010 03:45 PM by
NPehrsson

Yeah get rid of the ugly out parameter and rename it to something else.

It were quit close to the solution I provided :)

Comments have been closed on this topic.