A refactoring example: lots of if-else statements on strings

Sep 24th, 2012

I recently did a bit of work that turned out to be a great exercise
for refactoring a huge sequence of if-else statements based on
strings. There are a few ugly bits left, so I’m still poking at it,
but I am pleased with my progress so far.

While the original code was Java, the meat of the problem can be
easily shown in Ruby. Translating it to Ruby also makes it easier to
make sure I don’t accidentally share any proprietary information!

The problem involves processing a hunk of XML to create nested
configuration objects. The original implementation used a sequence of
if-else blocks, but the Ruby version would have used a case
statement.

12345678910111213141516171819202122

classObjectsFromXMLdefprocess(parent,element)new_object=nilcaseelement.namewhen'foo'f=Foo.new(element['name'])parent.add(f)new_object=fwhen'bar'b=Bar.new(element['name'])b.weight=element['weight'].to_fparent.add(b)new_object=b# ... about 20 of these cases in totalelseraise"Invalid node"endelement.children.each{|c|process(new_object,c)}endend

This function has some issues: it is pretty long, and each case has
some similarity to the next but are different enough to be
annoying. The code certainly doesn’t try to adhere to the Single
Responsibility Principle!

My first step was to split the blocks into classes with a common interface.

This corrals the item-specific code to an item-specific class. If I
need to change how the Bar class is created, only the BarHandler class
needs to be updated.

123456789101112131415161718192021222324

classFooHandler# As above...endclassBarHandler# As above...endclassObjectsFromXMLattr_reader:handlersdefinitialize@handlers={}handlers['foo']=FooHandler.newhandlers['bar']=BarHandler.new# ... other casesenddefprocess(parent,element)handler=handlers.fetch(element.name){raise"Invalid node"}new_object=handler.process(parent,element)element.children.each{|c|process(new_object,c)}endend

Now all the handlers are in a hash, keyed by the expected element
name. This allows me to pull out the correct handler and go. The
process function now only needs to be concerned with picking the
right handler and dealing with children elements.

classFooHandler# As above...defelement_name'foo'endendclassBarHandler# As above...defelement_name'bar'endendclassHandlersdefinitialize@handlers={}enddefadd(handler)@handlers[handler.element_name]=handlerenddefprocess(parent,element)handler=handlers.fetch(element.name){raise"Invalid node"}handler.process(parent,element)endendclassObjectsFromXMLattr_reader:handlersdefinitialize@handlers=Handlers.newhandlers.add(FooHandler.new)handlers.add(BarHandler.new)# ... other casesenddefprocess(parent,element)new_object=handlers.process(parent,element)element.children.each{|c|process(new_object,c)}endend

Now we have moved the concept of the expected element name into the
handler itself. This makes sense, as each handler should know what
name it expects, not some other piece of code. I also took the
opportunity to move the code purely related to the handlers to a new
class that is highly focused on that one responsibility.

Some further refinements happened after this last point. The
ObjectsFromXML class became another Handler class, which made it
the same level of abstraction as the other handlers and removed a
redundant process method. The return code was removed because it
wasn’t used except in a few tests. Iterating over children was moved
to each class that could contain children.