Recently, I was discussing with another programmer the best way to refactor a huge(1000 lines) method full of "if" statements.
The code is written in Java, but I guess this issue could happen in other languages such as C# as well.
To solve this problem, he suggested using a chain-of-responsibility pattern.
He proposed having a base "Handler" class. Then, "Handler1", "Handler2", etc. would extend "Handler".
Then, handlers would have a "getSuccessor" method, which would either return null(if it was the last of the chain) or the next Handler of the chain.
Then, a "handleRequest(Request)" function would either deal with Request, or pass it to the next of the chain and, if none of the previous solutions worked, it would return just null or throw an exception.
To add a new Handler to the chain, the coder would go to the last element of the chain and tell it there was a new element. To do something, he'd just call handleRequest on the first element of the chain.
To solve this problem, I suggested using a different approach.
I'd have a base "Handler" class as well, with "Handler1", "Handler2", just like the previous method mentioned.
However, there would be no "getSuccessor" method. Instead, I'd have a Collection class with a list of handlers(a Vector, an ArrayList, or whatever is best in this case).
The handleRequest function would still exist, but it wouldn't propagate the call to the next handlers. It would just process the request or return null.
To handle a request, one would use
for(Handler handle : handlers){
result = handle.handleRequest(request);
if(result!=null) return result;
}
throw new CouldNotParseRequestException(); //just like in the other approach
Or, to prevent code duplication, a "parseRequest(request)" method could be added to the collection class.
To add a new handler, one would go to the collection constructor(or static{} block, or something equivaleng) and simply add the code "addHandler(new Handler3());".
Exactly what advantages of chain-of-responsibility am I missing with this approach? Which method is best(assuming there is a best method)? Why? What potential bugs and issues can each design method cause?
For those who need context, here is what the original code looked like:
if(x instanceof Type1)
{
//doSomething1
} else if(x instanceof Type2)
{
//doSomething2
}
//etc.
I like your idea with collection better than those successors. It makes it easy and clear to manipulate this set of handlers: the collections interface is well known and everybody understands how to iterate over a List or what not.
If you use this successor way suggested by a friend, take care not to fall into a very deep recursion (unless your platform supports tail calls, I don't know if JVMs are capable of that).
I wouldn't recommend adding any methods to the collection. You get much more complicated design that's harder to comprehend and harder to modify. There are two separate concerns: storing a set of handlers and the interpretation of this handlers as a chain of responsibility. A method that handles requests by iterating over a collection is on higher level of abstraction than collection housekeeping methods, therefore shouldn't belong to collection interface.
Which approach is best depends on what your handlers want to do.
If the handlers can completely handle a request request on their own, your approach is fine. The handlers do not have a reference to other handlers, which makes the handler interface simple. Unlike the standard implementation of Chain of Responsibility, you can add or remove handlers from the middle of the chain. In fact, you can choose to build different chains depending on the type of request.
One problem with your approach is that a handler cannot do pre-processing or post-processing on the request. If this functionality is required, then Chain of Responsibility is better. In CoR, the handler is the one responsible for delegating to the next handler on the chain, so the handler can do pre-processing and/or post-processing, including modifying or replacing the response from the next handler on the chain. In this way, CoR is very similar to Decorator; it's just the intent that's different.
Since in CoR, the handler keeps a reference to the next item on the chain, you cannot add or remove items from the middle of the chain. A variation of CoR that allows you to add or remove items from the middle of the chain is a Filter Chain (see, for example, javax.servlet.FilterChain).
The code example you showed was a bunch of "if" statements that did different behavior based on the type of an object. If that is typical for the code you are cleaning up, you can simply have a map from the request type to the handler required.
Another approach to removing "if" statements is inheritance. If you had some behavior that you needed to do, and there was one variation for a web server, and other variation for a SOAP server, you could have a WebServerRequestHandler and a SoapServerRequestHandler, each extending RequestHandler. The advantage with inheritance is there a clearer place to put logic that is common to both types of request. The disadvantage is that since Java doesn't have multiple inheritance, you can only model single-dimensional problems.
Can't tell if Chain of Responsibility is your answer, or even if GoF applies. Visitor might be the right thing. Not enough information to be certain.
It could be that your problem can be handled with good old-fashioned polymorphism. Or maybe a Map that used keys to pick out the appropriate handler object.
Keep 'do the simplest thing possible' in mind. Don't leap to the complex until you've proven to yourself that you need it.
I recently read about the Anti-IF campaign that promotes this idea. Sounds quite pertinent here.
Go for CoR, if One of below satisfies:
- Your handlers depend on each other.
- You do not need parallelism between your handlers.
- You need ordering between these handlers
- If you want to return from say, the intermediate handler and do not want to process the rest of the handlers. It makes much more sense to use CoR, as you do not need to write
ifs inside for loop. You can instead just return from the current chain instead of going next in the chain.
