5
\$\begingroup\$

I've got a problem where I need to parse a line in a parameter file of the form

PotentialName [... variable number of parameters]

I've got code which works however it seems like it will be horrendous to maintain when I add more than a few options here.

What would be the best design pattern to avoid the long chain of else ifs and what looks like (to my eyes) quite a bit of boilerplate for each option I add.

std::shared_ptr<Potential> construct_potential( std::istringstream& iss )
{
    std::string pot_type;
    iss >> pot_type;

    if( pot_type == "NFW" ) {
        double rho0, rs;
        iss >> rho0 >> rs;
        return std::make_shared<NFW>(rho0, rs);
    }
    else if( pot_type == "MiyamotoNagai" ) {
        double m, a, b;
        if( iss >> m >> a >> b ) {
            return std::make_shared<MiyamotoNagai>(m, a, b);
        } else {
            throw ParseError("Expected 3 parameters for " + pot_type + " potential");
        }
    }
    else if( pot_type == "Kepler" ) {
        double m;
        if( iss >> m ) {
            return std::make_shared<Kepler>(m);
        } else {
            throw ParseError("Expected 1 parameter for " + pot_type + " potential");
        }
    }
    else {
        throw ParseError("Unknown Potential: " + pot_type);
    }
}

How can I improve this?

\$\endgroup\$
4
\$\begingroup\$

The approach to doing this is two-fold. Firstly, you'll want to modify your classes to have a constructor taking an std::istringstream& as an argument. The second part will be to make a map of strings to constructor functions. So let's assume our (skeleton) classes look something like:

class Potential
{
public:

    virtual ~Potential() { }

    virtual void print() const = 0;

};

class NFW
  : public Potential
{
private:

    double rho0;
    double rs;

public:

    NFW(std::istringstream& iss)
    {
        iss >> rho0 >> rs;
    }

    virtual ~NFW() = default;

    virtual void print() const override
    {
        std::cout << "NFW: rho0: " << rho0 << ", rs: " << rs << "\n";
    }
};

class MiyamotoNagai
  : public Potential
{
private:

    double m, a, b;

public:

    MiyamotoNagai(std::istringstream& iss)
    {
        iss >> m >> a >> b;
    }

    virtual ~MiyamotoNagai() = default;

    virtual void print() const override
    {
        std::cout << "MiyamotoNagai m: " << m << ", a: " << a 
                  << ", b: " << b << "\n";
    }
};

We then define the type our map will hold: it will hold a function that will return a std::shared_ptr<Potential>, and will take a std::istringstream& as an argument:

using function_t = std::function<std::shared_ptr<Potential>(std::istringstream&)>;

Using this, our map becomes:

std::unordered_map<std::string, function_t> register_map;

To add to this, we just need some very simple lambda functions that forward to the corresponding constructor:

register_map["NFW"] = [](std::istringstream& iss) { return std::make_shared<NFW>(iss); };
register_map["MiyamotoNagai"] = [](std::istringstream& iss) { return std::make_shared<MiyamotoNagai>(iss); };

The function to actually create our Potential subclass then looks like:

std::shared_ptr<Potential> create_potential(std::istringstream& iss)
{
    std::string potential_type;
    iss >> potential_type;
    auto it = register_map.find(potential_type);
    if(it == register_map.end()) {
        throw ParseError("Unknown Potential " + potential_type);
    }
    return it->second(iss);
}

With this, each constructor will need to do its own checking and error handling (which I've omitted above). As written, this better adheres to the open-closed principle (code should be open for extension, closed for modification), as the only thing you should have to touch when you create another subclass is adding another call to register_class[...].

A test example:

int main()
{
    register_map["NFW"] = [](std::istringstream& iss) { return std::make_shared<NFW>(iss); };
    register_map["MiyamotoNagai"] = [](std::istringstream& iss) { return std::make_shared<MiyamotoNagai>(iss); };
    std::istringstream iss("NFW 5 4");
    auto test = create_potential(iss);
    test->print();
}

As a final point, make absolutely sure this needs a shared_ptr over a unique_ptr. Without context it is impossible to tell, but you should always reach for unique_ptr first, and shared_ptr only if it is absolutely necessary; it may be in this case, but it is still something to very carefully think about.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Thank you, thats working perfectly. And you were right I didn't need a shared_ptr I only needed a unique_ptr which has increased the speed of my code to boot! \$\endgroup\$ – Simon Gibbons Feb 2 '15 at 15:41
4
\$\begingroup\$

I would suggest you to use Factory Method pattern here:

  • Define interface for construction of your Potential (i.e. PotentialCreator).
  • Define concrete implementations for each supported potential (KeplerPotentialCreator, MiyamotoNagaiPotentialCreator, etc).
  • Register pairs id-implementation in some entity, available during runtime.

Change your creator to something like this:

std::shared_ptr<Potential> construct_potential( std::istringstream& iss ) {
    std::shared_ptr<PotentialCreator> concreteCreator = registry.get(iss);
    if(!concreteCreator) {
        throw ParseError("Unknown Potential: " + pot_type);
    }
    return concreteCreator->construct();
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.