Refactor This!
The following method is ugly, what kind of refactoring would you do here?
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) { logger.Error("An error occured when raising the MessageCompleted event, the error will NOT affect the message processing", e); } return; } catch (Exception e) { logger.Warn("Failed to complete transaction, moving to error mode", e); exception = e; } } 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); } if (message == null) return; 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); } try { var copy = MessageProcessingFailure; if (copy != null) copy(currentMessageInformation, exception); } catch (Exception moduleException) { logger.Error("Module failed to process message failure: " + exception.Message, moduleException); } if (messageQueue.IsTransactional == false)// put the item back in the queue { messageQueue.Send(message); } }
Note, please use pastebin.com/ instead of posting code in the comments.
Comments
Det test on txDisposed == false in the second block is not needed, because if you reach the part where you set txDisposed to true, you will never reach the second block because of the return statement in the first block
1.This method has 6 parameters, to many.
2.We can add a finally clause to dispose transaction
3.Extract a method for "messageCompleted"
Obs. This refactoring it will be useful if we can see more code.
I don't see your point, this method is just ridiculously too complex. I wouldn't even spend time to seek if some simplifications could be done in the method itself.
1) Split it in several methods with relevant names to have at most one try/catch per method
2) Maybe see if some of these methods are similar
3) Having 6 parameters represent a whole context. It might be needed creating a complete class to handle the context, and put all smaller methods in the class.
Firstly, I agree 100% with Patrick Smacchia's comment.
I very often write this kind of method and call it (in my head) a 'script method'. It's a set of actions which must be performed in sequence.
I always refactor 'script methods' in the same way: Put each action in its own (well-named) method and turn the original method into a simple sequence of calls to the methods which do real work.
Omer,
I REALY don't like the throw exception for exception != null, it will cause misleading errors in the log, for one thing.
And it is still a pretty big & complex method.
Leonard,
disposing a transaction may throw, and we need to deal with that part specifically.
Patrick,
What do you mean, not see my point?
Don't you think this code is ugly? Don't you think that it should be refactored?
And 1 try/catch will absolutely not work, there are several layers here that you need to deal with.
I would start with overloading this method so that I can require a transaction scope to always be passed in. The overload would get the transaction in a using block.
I agree this function is over complicated, this code looks like a byproduct of bad design and no AOP.
you could reduce the code by making this pattern:
try
a function which recives the callbable,parameters and error message.
it will make 27 lines become 3 lines.
Use a behavior chain similar to the way that FubuMVC executes their controller action. This way the BeforeTransactionComplete that was added specifically for Linq DataContext only gets registered as a behavior for things. It really simplifies each behavior into something very manageable.
Using a behavior chain would also allow for a lot more reuse of code than it currently does. Each transport has very similar behavior in this area, but is for the most part copied between them. The chain could reuse existing behaviors if it makes sense to do so.
Oren:
I wouldn't mind much about throwing exceptions for the sake of keeping the code clear, but I understand your reasoning.
How about this approach:
http://pastebin.com/qAqJeqg6
I think it's clearer.
What I meant by 'I don't see the point' is I don't see the point in trying to understand the logic of such flawed piece of code and try to make it easier. It is too error prone and time consuming.
The only way to deal with such low quality code, is to split that in smaller methods with relevant names and eventually create a class to keep the context. This is purely mechanical, one doesn't need to think for that.
Once everything is splited, then it might be time to try to be smart and see if smaller methods can be refactored somehow.
I'd rename the method to "handleMethodcompletion" to adhere to my code guidelines. Apart from that it's just fine.
Oops, 1st of April is already over?
In that case I second Patrick's opinion.
Oren, I believe you are beginning to harness your readers as amazon mechanical turk employees..
Josh, this is cloud computing at work ;)
And regarding the method, if it's ugly and has ugly parameters, its callers must be also a bit ugly.
Omer,
You aren't handling the case where an error was passed to this function
Josh Schwartzberg,
While I certainly do that sometimes, this is not one of those times.
This refactoring was done before I posted this post, and you can look at the results at GitHub if you like, to see my answer.
[ICR],
Now that is a pretty refactoring.
Patrick,
"flawed piece of code" - it works, it is in production, it handles a very complex scenario very well.
90% of the refactoring that were tried broke something essential when they did that.
No, it isn't mechanical at all
This piece of code is flawed in the sense that it is unmaintainable. It is so unmaintainable that it became a blog post challenge to refactor it.
This is the exact definition of fragile code, it works but any attempt to make it nicer breaks it.
I never take a complex scenario for granted. A too complex scenario is just a set of easy scenarios that hasn't been refined enough. First 'refactor' the scenario itself, invent new artefacts, new concepts, new steps, new states, that add sense. Then the implementation that will follow will be easy to read and to maintain despite that fact that it will certainly be bigger.
"you can look at the results at GitHub if you like"
What repository? I can't seem to find it, though that may just be my unfamiliarity with GitHub.
Corey's approach is the best hands down in my opinion. That's how I would have done it, he beat me to it.
I found this repository git://github.com/ayende/rhino-esb.git, but the new refactoring is not commited there.
Leonard,
Opps, they are there now.
Where are the tests? How can anyone refactor this without tests? How would one ensure that it would still work after refactor?
Don't "Refactor this!". Please "Rewrite this" more like.