CODEWITHSUNDEEP
c++memory-leaksfactory-pattern
SimpleMind
SimpleMind

Reputation: 29

Leaking memory with a map using the factory pattern, solution?

class Factory
{
    public:
        Base* create()              { return new Rand();}
        Base* create(double value)  { return new Op(value); }
        Base* create(const std::string& comp, Base* x, Base* y) 
        {
            /*
            std::map<std::string, Base*> map_CreateFuncs = {
                { "+", new Add(x,y) },
                { "-", new Sub(x,y) },
                { "*", new Mult(x,y) },
                { "/", new Div(x,y) },
                { "**", new Pow(x,y) }
            };
            return (map_CreateFuncs.count(comp) == 1) ?  map_CreateFuncs[comp] : nullptr;
            */
                 if (comp == "+")  return new Add(x,y);
            else if (comp == "-")  return new Sub(x,y);
            else if (comp == "*")  return new Mult(x,y);
            else if (comp == "/")  return new Div(x,y);
            else if (comp == "**") return new Pow(x,y);
            else return nullptr;
        }
};

Using the if/elseif's works ~ no memory leaks. But I'd like to use the map method that I started with.

Wondering what the proper way to implement my create(string, Base*, Base*) is, using the map.

I've read code out there that uses a typedef but I couldn't understand its implementation.

Upvotes: 0

Views: 158

Answers (1)

stiar
stiar

Reputation: 368

  1. Unless there is a very specific reason to use raw pointers, consider always using std::unique_ptr by default. Using smart pointers allows to both avoid memory leaks and clearly document the ownership model. For example, when you create Add(x, y), should the ownership of x and y be transferred to the newly created object, or the ownership might be shared with some other instances? In case of the latter, consider std::shared_ptr.

  2. You probably don't want to create class instances for all of your available operations every time. Instead, you could introduce lazy evaluation by storing an object that maps operations to factory functions that create appropriate class instances.

Overall, this would look somewhat like this:

class Base {
public:
    virtual ~Base() = 0;
};

class Add : public Base {
public:
    static std::unique_ptr<Base> create(std::unique_ptr<Base> x, std::unique_ptr<Base> y);

private:
    // Constructor is private so that the user can only create smart pointers holding instances of Add.
    Add(std::unique_ptr<Base> x, std::unique_ptr<Base> y);
};

class Factory
{
public:
    std::unique_ptr<Base> create(const std::string& comp, std::unique_ptr<Base> x, std::unique_ptr<Base> y) 
    {
        using FactoryMethod = std::function<std::unique_ptr<Base>(std::unique_ptr<Base>, std::unique_ptr<Base>)>;
        static std::map<std::string, FactoryMethod> map_CreateFuncs = {
            { std::string("+"), FactoryMethod([](std::unique_ptr<Base> x, std::unique_ptr<Base> y){
                return Add::create(std::move(x), std::move(y)); })},
            // Other operations ...
        };

        const auto factory_method_it = map_CreateFuncs.find(comp);
        if (factory_method_it == map_CreateFuncs.end()) {
            return nullptr;
        }
        return (factory_method_it->second)(std::move(x), std::move(y));
    }
};

Keep in mind that std::map and std::function might introduce some overhead over simple if-else clauses.

Upvotes: 1

Related Questions