Worst Code Ever!

2009 June 16
by Nick Lamb

First take a look at what the offending code that I have to maintain looks like, then I’ll explain.

public IList<Product> ListProducts(string filter)
{

IList<Exception> exceptionResults = new List<Exception>();

List<List<Product>> products =

handlerMultiCaster.Handle<List<Product>>(out

exceptionResults, “TRD.LISTPRODUCTS”, distributionPoint, filter);

if (products.Count == 1)
{

return products[0];

}
return
null;

}

So the first thing to notice about this code is that there are two main concepts, a “multi caster” and a “handler”.  “Handlers” have only one method called Handle() and you pass a magic string telling it what exactly to “handle”. This in itself is terrible because a developer has no idea what the possible values of what they should pass are unless they dig down into the Handle method to find where the other magic strings are stored.  “Handlers” are configured through an arcane configuration framework that loads .dlls with concrete implementations of the Handler that have such methods as Invoke() and DoHandle().

Now it gets even better with the “multi caster”.  In an application there may be any number of “Handlers” configured and since these handlers get invoked dynamically(something any other dependency injection framework could have done better) the best thing that the “multi caster” can do is to loop over all configured handlers and call Handle on ALL of them(whether they’re actually going to work or not).  This is the really hilarious part where the IList of Exceptions comes into play.  Since the multi caster just calls all the handlers blindly most are bound to fail and throw exceptions(there goes performance), so it returns a list of all those exceptions.  What to do with all those exceptions is anyone’s guess.  What’s worse is when the so called multi caster rips through each handle method some of them could get part way through execution and then throw leaving multiple handlers in an unkown state, perfect!  The last part is just really annoying, if you only need one list of products, why return a list of lists of products and then just to add salt to the wound by only grabbing the first list out of the list of lists.

There are so many anti-patterns here it makes my head spin, but nobody else seems to think it’s a backwards way to accomplish something.

  • http://kirstenlamb.net Kirsten

    You lost me at “IList.”

    But in order to make the most of this comment, I suggest you place periods inside quotes and spaces before opening parenthesis. And one space after periods/exclamation points.

  • http://acdub.com Andy White

    You had me at HandlerMultiCaster… Blech… I can count at least 5 horrendous things in just the first few lines of that, and I can’t bear to read the rest.

  • http://www.OdysseyIsland.com Jason Short

    Oh man, it’s nice to see things got better after I left. I believe you forgot to mention the Federation! As it was explained to me, once you introduce the “magic string” and injection it becomes “Federated”. Okay, time for beer.

  • Sean Williams

    Mind… Numb… no more handlers!