Getting rid of procedural code

I came across a piece of code similar to the following this week:

if(cmdLine[0] == "-option1")
{
    // load some objects from XML
}
else if(cmdLine[0] == "-option2")
{
    // load some objects from database
}
...
if(cmdLine[0] == "-option1")
{
    // lots of code to save to xml here
    // manipulating the DOM directly
}
else if(cmdLine[0] == "-option2")
{
    // code to save to database here
}
...

The idea behind this is that the application was supposed to keep backwards compatibility with another system that used XML files to pass data back and forth and also support a newer version of that system, that uses special database tables to do that.

So what’s wrong with it? Well, not taking repetition of similar blocks (this if statement occurs many many times in the code) into account and even closing one eye at the need to refactor this code into smaller methods, there’s still one bad thing left.

This code is procedural.

Adding New Requirements

You can see that the code above switches methods of receiving and transferring data to another system, depending on command line parameters used. What if later we get a new requirement to use flat files for storing the data, by keeping the backwards compatibility? And then later, we’ll need to support sending data through web service. And later… I hope you see the point.

With the method shown above, every new way of data transferring will require a new “else if(…)” branch with more code inside. Thus, we have to change the code and risk breaking it with every new requirement.

How to Refactor this Code?

No, using case statement won’t make it any better. That’s just syntactic change. The answer here is inheritance. Yes, that means we’ll create a couple of new classes (that’s how we solve problems in object-oriented languages, isn’t it?).

Let’s start by defining what the code should do. It should provide a means to load and save objects as part of the integration process. We already know that there are different kinds of implementations needed – XML and database based – so we can use Strategy Pattern here. Let’s see what the interface of these classes looks like in our case:

public interface IIntegrationStrategy
{
    object LoadObject();
    void SaveObject(object someObject);
}

That’s the interface that we’ll program against. The code using it will have no idea what persistence medium the implementation is using. The first example in this post would then be rewritten like this:

...
object someObject = integrationStrategy.LoadObject();
...
integrationStrategy.SaveObject(someObject);
...

It’s that simple! There’s no conditional logic – it is moved into a factory that decides what implementation of IIntegrationStrategy to create for application’s needs:

public class IntegrationStrategyFactory
{
    public IIntegrationStrategy CreateByCommandLineParameter(string parameter)
    {
        if(parameter == "-option1")
            return new XmlIntegrationStrategy();  // XML-based implementation
        else if(parameter == "-option2")
            return new DatabaseIntegrationStrategy();  // database based implementation
 
        // if parameter is not recognized, return a stub object that does nothing
	// - implementation of Null Object pattern
        return new NullIntegrationStrategy();
    }
} 

At some point in our application (in the Main method, for example), we’ll have code like this:

IIntegrationStrategy integrationStrategy = 
	new IntegrationStrategyFactory().CreateByCommandLineParameter(cmdLine[0]);
MainForm form = new MainForm(integrationStrategy);  // the integration service 
				// is injected into the form's constructor

Now, whenever we need to add a new integration strategy, we implement the interface and change code at one place only – the factory. The code that used the interface remains untouched and works fine. This is the point of the Open/Closed Principle – we extend the behavior of our application without modifying the existing code.

CodeProject