Factory pattern using unique_ptr in c++

Question

I have an old factory implementation in c++, and I want to use unique pointers instead of raw pointers in it. A minimal example of my code is as follows. I have a base class A, and a derived class B. In main(), I pass 1 to the create method in A, and the type of b1 is now changed to B.

#include <iostream>
#include <map>

class A {
 public:
  A() {}
  virtual void Foo() {}
  std::map<int, A *> &registerType() {
    static std::map<int, A *> map_instance;
    return map_instance;
  }
  A *create(int n) { return registerType()[n]; }
};

class B : A {
 public:
  B() { registerType()[1] = this; }
  void Foo() { std::cout << "I am B!\n"; }
};

static B b0;

int main() {
  A *b1 = new A();
  b1 = b1->create(1);
  b1->Foo();

  return 0;
}

Now if I want to change raw pointers to unique pointers, I naturally get a collection of errors (the following code results in errors):

#include <iostream>
#include <map>
#include <memory>

class A {
 public:
  A() {}
  virtual void Foo() {}
  std::map<int, std::unique_ptr<A>> &registerType() {
    static std::map<int, std::unique_ptr<A>> map_instance;
    return map_instance;
  }
  std::unique_ptr<A> create(int n) { return registerType()[n]; }
};

class B : A {
 public:
  B() { registerType()[1](this); }
  void Foo() { std::cout << "I am B too!\n"; }
};

static B b0;

int main() {
  std::unique_ptr<A> b1(new A());
  b1 = b1->create(1);
  b1->Foo();

  return 0;
}

The errors are:

In member function 'std::unique_ptr<A> A::create(int)':
use of deleted function 'std::unique_ptr<_Tp, _Dp>::unique_ptr(const std::unique_ptr<_Tp, _Dp>&) [with _Tp = A; _Dp = std::default_delete<A>]'
   std::unique_ptr<A> create(int n) { return registerType()[n]; }

In constructor 'B::B()':
no match for call to '(std::map<int, std::unique_ptr<A> >::mapped_type {aka std::unique_ptr<A>}) (B* const)'
   B() { registerType()[1](this); }
                               ^

So I want to know:

1. Were unique pointers intended to be used in cases like mine? (I assume the response should be yes!)
2. I need to pass this as a unique_ptr type to the registerType method. How I can pass the ownership of the pointer to the current instance (this keyword) to a unique_ptr? (If it is possible or was intended to be possible.)
3. If it is a good practice to use unique pointers here, how I should implement it?

Solution

First of all, if someone wants to implement a factory pattern, an acceptable way of doing it with raw pointers is as follows:

#include <iostream>
#include <map>

class A;

class A_Factory {
 public:
  A_Factory() {}
  virtual A *create() = 0;
};

class A {
 public:
  A() {}
  static void registerType(int n, A_Factory *factory) {
    get_factory_instance()[n] = factory;
  }
  static A *create(int n) {
    A *A_instance = get_factory_instance()[n]->create();
    return A_instance;
  }
  virtual void setMyID(int n) {}
  virtual void I_am() { std::cout << "I am A\n"; }
  virtual ~A() {}

 protected:
  int MyID;
  static std::map<int, A_Factory *> &get_factory_instance() {
    static std::map<int, A_Factory *> map_instance;
    return map_instance;
  }
};

class B : public A {
 public:
  B() {}
  void Foo() {}
  void I_am() { std::cout << "I am B " << MyID << "\n"; }
  void setMyID(int n) { MyID = n; }
  ~B() {}

 private:
};

class B_Factory : public A_Factory {
 public:
  B_Factory() { A::registerType(1, this); }
  A *create() { return new B(); }
};

static B_Factory b0_factory;

void caller() {}

int main() {
  A *b1 = A::create(1);
  A *b2 = A::create(1);
  b1->setMyID(10);
  b2->setMyID(20);
  b1->I_am();
  b2->I_am();
  delete b1;
  delete b2;

  return 0;
}

A is the base class, and B is the derived one. If we pass 1 to A::create(int n), an object of type B will be produced. The memory is managed manually and there would be no memory leak.

Concerning the questions in the post:

1. YES. unique_ptr is awesome; use them wherever you can!
2. With the design presented in the question, passing the ownership of this was somehow necessary. I cannot think of a way to pass the ownership of this. With the design presented in the answer, it is not necessary to pass the ownership of this.
3. Implement the unique_ptr in the above factory pattern as below:

#include <iostream>
#include <map>
#include <memory>

 class A;

 class A_Factory {
 public:
  A_Factory() {}
  virtual std::unique_ptr<A> create_unique() = 0;
};

 class A {
 public:
  A() {}
  static void registerType(int n, A_Factory *factory) {
    get_factory_instance()[n] = factory;
  }
  static std::unique_ptr<A> create_unique(int n) {
    std::unique_ptr<A> A_instance =
        std::move(get_factory_instance()[n]->create_unique());
    return A_instance;
  }

  virtual void setMyID(int n) {}
  virtual void I_am() { std::cout << "I am A\n"; }
  virtual ~A() {}

 protected:
  int MyID;
  static std::map<int, A_Factory *> &get_factory_instance() {
    static std::map<int, A_Factory *> map_instance;
    return map_instance;
  }
};

 class B : public A {
 public:
  B() {}
  void Foo() {}
  void I_am() { std::cout << "I am B " << MyID << "\n"; }
  void setMyID(int n) { MyID = n; }
  ~B() {}

 private:
};

 class B_Factory : public A_Factory {
 public:
  B_Factory() { A::registerType(1, this); }
  std::unique_ptr<A> create_unique() {
    std::unique_ptr<A> ptr_to_B(new B);
    return ptr_to_B;
  }
};

 static B_Factory b0_factory;

 void caller() {}

 int main() {
  std::unique_ptr<A> b1 = std::move(A::create_unique(1));
  std::unique_ptr<A> b2 = std::move(A::create_unique(1));
  b1->setMyID(10);
  b2->setMyID(20);
  b1->I_am();
  b2->I_am();

  return 0;
}

As you can see, no manual memory management is necessary and the memory management is handled by the unique_ptr.

Comments

[Captain Obvlious]

"I naturally get a collection of errors" - Please include the full text of the errors in your post. Feel free to omit duplicates that occur across multiple lines.

[WhozCraig]

I don't see how this would work with std::unique_ptr. You can certainly store them in a std::map, and you can reference them. But they are at-best made to be moved; not copied. Moving them out of your map would work, but then why bother having a map in the first place. It would seem a std::shared_ptr would pay better dividends for what you are trying.(which also will allow you to setup sharing this if properly configured.)

[PeterT]

@a.sam it's written in a really confusing way, you have a "create" function that doesn't create anything and are leaking memory by doing b1 = b1->create(1);

[a.sam]

@PeterT: +1 for your awesome point. So even if I add destructor to A and B, I cannot avoid memory leak. Am I right? So, how can I avoid memory leak in the first place?

[PeterT]

@a.sam if you new something delete it. For your specific case, don't create an A in the first place. Make both registerType and create static, since all they do is access a static data memeber and call it with A *b1 = A::create(1);

[MatG]

C.30: Define a destructor if a class needs an explicit action at object destruction "...If the default destructor is sufficient, use it. Only define a non-default destructor if a class needs to execute code that is not already part of its members’ destructors."