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

Question

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.

Don't use `new`. Instead use [`std::unique_ptr`](https://en.cppreference.com/w/cpp/memory/unique_ptr). It manages the memory for you, so no leaks. — NathanOliver, Mar 02 '21 at 21:55
If you don't use `unique_ptr` you'll have to figure out when you are not using the `map` anymore and `delete` all of the objects you put in it. Determining when it's safe to `delete`... that's one of the more annoying programming problems, so use `unique_ptr` and other smart pointers to figure it out for you and ensure the deed gets done. — user4581301, Mar 02 '21 at 21:59
Regarding `return (map_CreateFuncs.count(comp) == 1) ? map_CreateFuncs[comp] : nullptr;` you can save yourself a look-up. `map_CreateFuncs.count(comp) == 1)` searches the map. `map_CreateFuncs[comp]` also searches the map. `auto found = map_CreateFuncs.find(comp);` is one search. Then you `return found != map_CreateFuncs.end()? *it : nullptr;` — user4581301, Mar 02 '21 at 22:05
That said, this probably isn't what you want to do at all, not if you want to return a new `Base` derived object every time as one would expect from a factory. Need to see more of your use-case to make a decent answer, though. — user4581301, Mar 02 '21 at 22:07
In the specification we were given, we are expected to return a nullptr if input is invalid, so I handle the case of the nullptr in the client/ main() — SimpleMind, Mar 02 '21 at 22:26

score 1 · Accepted Answer · answered Mar 02 '21 at 22:29

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.
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.

Note: `x` and `y` would probably be better suited to passing as references than pointer or `unique_ptr`. But why are the operands also derived from `Base`? Could be looking at a problem with the [Liskov Substitution Principle](https://stackoverflow.com/questions/56860/what-is-an-example-of-the-liskov-substitution-principle) in the offing — user4581301, Mar 02 '21 at 22:44
In searching google, I did find many made use of static create methods in each derived function. I was avoiding that with my implementation. std::function is new to me and I think that takes care of what I was looking for, thanks so much for that! — SimpleMind, Mar 02 '21 at 22:45
@user4581301 you're probably right about that, I'll experiment to see if I can just pass them by reference. Im not familiar with that substitution principle, so thank you for the link! As for why its derived from Base; Base is just an abstract class from which Add/Subtract/Multiple/Divide/Power inherit from ~ this was initially done for the composite pattern, now its using the same elements but with the Factory pattern. — SimpleMind, Mar 02 '21 at 22:48
I'd post my source but I can only imagine how bad it'd be received ^^; The parser in particular is quite ugly — SimpleMind, Mar 02 '21 at 22:49
Hmm, I dont think I can pass x, y by reference -- since they inherit from Base and thus can be a composite. It basically becomes something of a tree where each operator is a node with lhs/rhs. Base can be an operator or operand where lhs/rhs are nullptr's. — SimpleMind, Mar 02 '21 at 22:53
It does not seem to be a good idea to pass them as references. I do not know more details about Base and the classes for operations, but it looks like the intention is to own the passed parameters to be able to use them later for evaluation of the expression, and the passed references may become dangling. — stiar, Mar 02 '21 at 22:55

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

1 Answers1