Refactor ThisResults
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
Why not put txDisposed as a member of the class? That would avoid the ugly out parameter.
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.
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
Yeah get rid of the ugly out parameter and rename it to something else.
It were quit close to the solution I provided :)
Comment preview