Refactoring repeated If statement blocks

Question

I have some code that I'm trying to rewrite. The code is designed to be "generic", in that it can be used by many different callers that require different "work flows". It goes something like this:

string globalA;
int globalB;
bool globalC;
// Lots more globals in various shapes and forms

void TheOneAndOnlyMethod(XmlBasedConfig config) {

    // Set all of the globals based on XML configuration
    // ...

    if (globalA.Length > 0)
        // Do something relating to functionality 'a'
    }

    if (globalB > 0) {
        // Do something relating to functionality 'b'
    }

    if (globalC) {
        // You get the idea
    }
}

Some callers will have globalA and globalB set, therefore doing whatever is in the related if blocks. Other callers will have a myriad of other settings to do whatever they need to do. Callers are basically just an XML file with settings.

Maintaining/modifying this code is a big pain. I know there must be a cleaner, simpler, less brain-exploding way to do this!

Answer 1

I would start by turning each if statement into a method with a name reflecting what it does. Then, instead of setting globals based on the XML file contents, parse the file and call the appropriate methods in order rather than communicating through variables.

Answer 2

The name of your method gives some insight into the problem/solution: TheOneAndOnlyMethod. It sounds like you need to break your code into many smaller methods that each handle very specific operations and are also reusable.

string globalA;
int globalB;
bool globalC;
// Lots more globals in various shapes and forms

void TheOneAndOnlyMethod(XmlBasedConfig config) {
    // Set all of the globals based on XML configuration
    loadXmlAsGlobals(config);

    if (globalA.Length > 0)
        methodOne(globalA);
        methodTwo(globalA);
    }

    if (globalB > 0) {
        methodTwo(globalB);
        methodThree(globalB);
    }

    if (globalC) {
        methodOne(globalC);
        methodFour(globalC);
    }
}

Answer 3

It depend of you're XML file structure. If I could access A/B/C/... seperately my c++/boost code would look like this.

Refactor out all the A related stuff in class FunctionalityA , B related stuff in class FunctionalityB, ... FunctionalityProvider class is the class where you configure the functionality of your system. TheOneAndOnlyMethod asks the provider for all the functionality and iterates over them.

class XmlFunctionality
{
public:
    virtual ~XmlFunctionality(){
    }
    virtual void loadFromConfig(XmlBasedConfig) = 0;
    virtual bool isEnabled() const = 0;
    virtual void execute() = 0; 

protected:
    XmlFunctionality(){
    };
}

class FunctionalityA : public XmlFunctionality
{
public:
    void loadFromConfig(XmlBasedConfig){
        // load A information from xml
    }
    bool isEnabled() const{
        return globalA.length()>0; // is A a global !?    
    }
    void execute(){
        // do you're a related stuff
    }
}

class FunctionalityB : public XmlFunctionality
{
public:
    void loadFromConfig(XmlBasedConfig){
        // load B information from xml
    }
    bool isEnabled() const{
        // when is b enabled ...    
    }
    void execute(){
        // do you're b related stuff
    }
}

// Map your question to the functions - 
class FunctionalityProvider
{
    Functionality functionalityList;
public:
    typedef std::vector<XmlFunctionality*> Functionality; 
    FunctionalityProvider() : functionalityList() {        
        functionalityList.push_back( new FunctionalityA);
        functionalityList.push_back( new FunctionalityB);
        functionalityList.push_back( ... );
    }

    ~FunctionalityProvider {        
        for_each( functionality.begin(), functionality.end(), delete_ptr() );
    }

    Functionality functionality() const {
        return functionalityList;
    }
}
void TheOneAndOnlyMethod(XmlBasedConfig config, const FunctionalityProvider& provider) {
    const FunctionalityProvider::Functionality functionality = provider.functionality();
    for_each( 
        functionality.begin(), 
        functionality.end(), 
        bind(&XmlFunctionality::loadFromConfig, _1, config)); // or some other way of parsing the file
    for_each( 
        functionality.begin(), 
        functionality.end(), 
        if_then( bind(&XmlFunctionality::isEnabled, _1), bind(&XmlFunctionality::execute, _1)));

}

If I couldn't access A/B/C seperately I let a parser returing a list of Functionality based on the content of the XML file.