Refactor This (Ugly Code)!
- by Alois Kraus
Ayende has put on his blog some ugly code to refactor. First and foremost it is nearly impossible to reason about other peoples code without knowing the driving forces behind the current code. It is certainly possible to make it much cleaner when potential sources of errors cannot happen in the first place due to good design. I can see what the intention of the code is but I do not know about every brittle detail if I am allowed to reorder things here and there to simplify things. So I decided to make it much simpler by identifying the different responsibilities of the methods and encapsulate it in different classes. The code we need to refactor seems to deal with a handler after a message has been sent to a message queue. The handler does complete the current transaction if there is any and does handle any errors happening there. If during the the completion of the transaction errors occur the transaction is at least disposed. We can enter the handler already in a faulty state where we try to deliver the complete event in any case and signal a failure event and try to resend the message again to the queue if it was not inside a transaction. All is decorated with many try/catch blocks, duplicated code and some state variables to route the program flow. It is hard to understand and difficult to reason about. In other words: This code is a mess and could be written by me if I was under pressure. Here comes to code we want to refactor: private void HandleMessageCompletion(
Message message,
TransactionScope tx,
OpenedQueue messageQueue,
Exception exception,
Action<CurrentMessageInformation, Exception> messageCompleted,
Action<CurrentMessageInformation> beforeTransactionCommit)
{
var txDisposed = false;
if (exception == null)
{
try
{
if (tx != null)
{
if (beforeTransactionCommit != null)
beforeTransactionCommit(currentMessageInformation);
tx.Complete();
tx.Dispose();
txDisposed = true;
}
try
{
if (messageCompleted != null)
messageCompleted(currentMessageInformation, exception);
}
catch (Exception e)
{
Trace.TraceError("An error occured when raising the MessageCompleted event, the error will NOT affect the message processing"+ e);
}
return;
}
catch (Exception e)
{
Trace.TraceWarning("Failed to complete transaction, moving to error mode"+ e);
exception = e;
}
}
try
{
if (txDisposed == false && tx != null)
{
Trace.TraceWarning("Disposing transaction in error mode");
tx.Dispose();
}
}
catch (Exception e)
{
Trace.TraceWarning("Failed to dispose of transaction in error mode."+ e);
}
if (message == null)
return;
try
{
if (messageCompleted != null)
messageCompleted(currentMessageInformation, exception);
}
catch (Exception e)
{
Trace.TraceError("An error occured when raising the MessageCompleted event, the error will NOT affect the message processing"+ e);
}
try
{
var copy = MessageProcessingFailure;
if (copy != null)
copy(currentMessageInformation, exception);
}
catch (Exception moduleException)
{
Trace.TraceError("Module failed to process message failure: " + exception.Message+
moduleException);
}
if (messageQueue.IsTransactional == false)// put the item back in the queue
{
messageQueue.Send(message);
}
}
You can see quite some processing and handling going on there. Yes this looks like real world code one did put together to make things work and he does not trust his callbacks. I guess these are event handlers which are optional and the delegates were extracted from an event to call them back later when necessary. Lets see what the author of this code did intend:
private void HandleMessageCompletion(
TransactionHandler transactionHandler,
MessageCompletionHandler handler,
CurrentMessageInformation messageInfo,
ErrorCollector errors
)
{
// commit current pending transaction
transactionHandler.CallHandlerAndCommit(messageInfo, errors);
// We have an error for a null message do not send completion event
if (messageInfo.CurrentMessage == null)
return;
// Send completion event in any case regardless of errors
handler.OnMessageCompleted(messageInfo, errors);
// put message back if queue is not transactional
transactionHandler.ResendMessageOnError(messageInfo.CurrentMessage, errors);
}
I did not bother to write the intention here again since the code should be pretty self explaining by now. I have used comments to explain the still nontrivial procedure step by step revealing the real intention about all this complex program flow. The original complexity of the problem domain does not go away but by applying the techniques of SRP (Single Responsibility Principle) and some functional style but we can abstract the necessary complexity away in useful abstractions which make it much easier to reason about it.
Since most of the method seems to deal with errors I thought it was a good idea to encapsulate the error state of our current message in an ErrorCollector object which stores all exceptions in a list along with a description what the error all was about in the exception itself. We can log it later or not depending on the log level or whatever. It is really just a simple list that encapsulates the current error state.
class ErrorCollector
{
List<Exception> _Errors = new List<Exception>();
public void Add(Exception ex, string description)
{
ex.Data["Description"] = description;
_Errors.Add(ex);
}
public Exception Last
{
get
{
return _Errors.LastOrDefault();
}
}
public bool HasError
{
get
{
return _Errors.Count > 0;
}
}
}
Since the error state is global we have two choices to store a reference in the other helper objects (TransactionHandler and MessageCompletionHandler)or pass it to the method calls when necessary. I did chose the latter one because a second argument does not hurt and makes it easier to reason about the overall state while the helper objects remain stateless and immutable which makes the helper objects much easier to understand and as a bonus thread safe as well. This does not mean that the stored member variables are stateless or thread safe as well but at least our helper classes are it.
Most of the complexity is located the transaction handling I consider as a separate responsibility that I delegate to the TransactionHandler which does nothing if there is no transaction or
Call the Before Commit Handler
Commit Transaction
Dispose Transaction if commit did throw
In fact it has a second responsibility to resend the message if the transaction did fail. I did see a good fit there since it deals with transaction failures.
class TransactionHandler
{
TransactionScope _Tx;
Action<CurrentMessageInformation> _BeforeCommit;
OpenedQueue _MessageQueue;
public TransactionHandler(TransactionScope tx, Action<CurrentMessageInformation> beforeCommit, OpenedQueue messageQueue)
{
_Tx = tx;
_BeforeCommit = beforeCommit;
_MessageQueue = messageQueue;
}
public void CallHandlerAndCommit(CurrentMessageInformation currentMessageInfo, ErrorCollector errors)
{
if (_Tx != null && !errors.HasError)
{
try
{
if (_BeforeCommit != null)
{
_BeforeCommit(currentMessageInfo);
}
_Tx.Complete();
_Tx.Dispose();
}
catch (Exception ex)
{
errors.Add(ex, "Failed to complete transaction, moving to error mode");
Trace.TraceWarning("Disposing transaction in error mode");
try
{
_Tx.Dispose();
}
catch (Exception ex2)
{
errors.Add(ex2, "Failed to dispose of transaction in error mode.");
}
}
}
}
public void ResendMessageOnError(Message message, ErrorCollector errors)
{
if (errors.HasError && !_MessageQueue.IsTransactional)
{
_MessageQueue.Send(message);
}
}
}
If we need to change the handling in the future we have a much easier time to reason about our application flow than before.
After we did complete our transaction and called our callback we can call the completion handler which is the main purpose of the HandleMessageCompletion method after all. The responsiblity o the MessageCompletionHandler is to call the completion callback and the failure callback when some error has occurred.
class MessageCompletionHandler
{
Action<CurrentMessageInformation, Exception> _MessageCompletedHandler;
Action<CurrentMessageInformation, Exception> _MessageProcessingFailure;
public MessageCompletionHandler(Action<CurrentMessageInformation, Exception> messageCompletedHandler,
Action<CurrentMessageInformation, Exception> messageProcessingFailure)
{
_MessageCompletedHandler = messageCompletedHandler;
_MessageProcessingFailure = messageProcessingFailure;
}
public void OnMessageCompleted(CurrentMessageInformation currentMessageInfo, ErrorCollector errors)
{
try
{
if (_MessageCompletedHandler != null)
{
_MessageCompletedHandler(currentMessageInfo, errors.Last);
}
}
catch (Exception ex)
{
errors.Add(ex, "An error occured when raising the MessageCompleted event, the error will NOT affect the message processing");
}
if (errors.HasError)
{
SignalFailedMessage(currentMessageInfo, errors);
}
}
void SignalFailedMessage(CurrentMessageInformation currentMessageInfo, ErrorCollector errors)
{
try
{
if (_MessageProcessingFailure != null)
_MessageProcessingFailure(currentMessageInfo, errors.Last);
}
catch (Exception moduleException)
{
errors.Add(moduleException, "Module failed to process message failure");
}
}
}
If for some reason I did screw up the logic and we need to call the completion handler from our Transaction handler we can simple add to the CallHandlerAndCommit method a third argument to the MessageCompletionHandler and we are fine again. If the logic becomes even more complex and we need to ensure that the completed event is triggered only once we have now one place the completion handler to capture the state.
During this refactoring I simple put things together that belong together and came up with useful abstractions. If you look at the original argument list of the HandleMessageCompletion method I have put many things together:
Original Arguments
New Arguments Encapsulate
Message message
CurrentMessageInformation messageInfo
Message message
TransactionScope tx
Action<CurrentMessageInformation> beforeTransactionCommit
OpenedQueue messageQueue
TransactionHandler transactionHandler
TransactionScope tx
OpenedQueue messageQueue
Action<CurrentMessageInformation> beforeTransactionCommit
Exception exception,
ErrorCollector errors
Action<CurrentMessageInformation, Exception> messageCompleted
MessageCompletionHandler handler
Action<CurrentMessageInformation, Exception> messageCompleted
Action<CurrentMessageInformation, Exception> messageProcessingFailure
The reason is simple: Put the things that have relationships together and you will find nearly automatically useful abstractions. I hope this makes sense to you. If you see a way to make it even more simple you can show Ayende your improved version as well.
