C++ static factory constructor

Question

I am in the process of making a simulation and it requires the creation of multiple, rather similar models. My idea is to have a class called Model and use static factory methods to construct a model. For example; Model::createTriangle or Model::createFromFile. I took this idea from previous java code and was looking for ways to implement this in C++.

Here is what I came up with so far:

#include <iostream>

class Object {
    int id;

public:
    void print() { std::cout << id << std::endl; }

    static Object &createWithID(int id) {
        Object *obj = new Object();

        obj->id = id;

        return *obj; 
    }
};

int main() {
    Object obj = Object::createWithID(3);

    obj.print();

    return 0;
}

Some questions about this:

• Is this an accepted and clean way of making objects?
• Does the returned reference always ensure correct removal of the object?
• Is there any way to do this without pointers?

Answer 1

Is this an accepted and clean way of making objects?

It is (unfortunately) accepted but it's not clean.

Instead of factory functions just use constructors.

That's what they're for.

Does the returned reference always ensure correct removal of the object?

The reference is irrelevant except to the extent that it misleads users of the function.

In your example the reference has apparently misled yourself into not destroying the dynamically allocated object, but just copying it.

Better return a smart pointer.

But as already mentioned, it's even better to ditch the idea of factory functions.

They're wholly unnecessary here.

Is there any way to do this without pointers?

No, not if "this" refers to the dynamic allocation, but you can and should use constructors instead of factory functions.

---

Example:

#include <iostream>

namespace better {
    using std::ostream;

    class Object
    {
    public:
        auto id() const -> int { return id_; }
        explicit Object( int const id): id_( id ) {}
    private:
        int id_;
    };

    auto operator<<( ostream& stream, Object const& o )
        -> ostream&
    { return (stream << o.id()); }
}  // namespace better

auto main()
    -> int
{
    using namespace std;
    cout << better::Object( 3 ) << endl;
}

Answer 2

Unless you are using polymorphism there is no reason for your factory functions to return any kind of pointer, they can just return the object by value. Any modern compiler will do return value optimization so there is no copy.

If you are after an "accepted and clean" way then that sounds quite opinion based and dependent on how this class will be used but what I would do is keep the definition of Model as small as possible. Only include what is needed for it to do its job with a minimum number of constructors required for normal usage:

namespace Simulation {
  class Model {
   private: 
    int id_;
   public:
    explicit Model(int id) : id_(id) {}

    // minimum required to do the job...
  };
}

Then, I would define the functions to create various flavors of Model separately. For example, as non-member, non-friend functions in a namespace:

namespace Simulation {  
  Model createTriangle(int id) {
    Model model(id);
    // do whatever you need to do to make it a triangle...
    return model;
  }

  Model createSquare(int id) {
    Model model(id);
    // do whatever you need to do to make it a square...
    return model;
  }  
}

That way, if you find you need another flavor of Model, you don't need to change the Model class. Your create functions can even be spread across multiple files if needed or become part of a Builder or Factory class. Usage would look like:

int main() {
  Simulation::Model m1(0);
  Simulation::Model m2 = Simulation::createTriangle(1);
  Simulation::Model m3 = Simulation::createSquare(2);
}

Answer 3

This is an absolutely terrible way to create your objects. Every time that createWithID is called, a new Object is constructed on the free store which is never able to be destroyed.

You should rewrite createWithID to:

static Object createWithID(int id) {
    Object obj;
    obj.id = id;
    return obj; 
}

Or better, you could just supply a constructor for your Object objects.

If you want to enable polymorphic objects, you should use something like wheels::value_ptr.

Answer 4

Your code currently contains a memory leak: any object created using new, must be cleaned up using delete. The createWithID method should preferably not use new at all and look something like this:

static Object createWithID(int id) 
{
    Object obj;
    obj.id = id;
    return obj; 
}

This appears to require an additional copy of the object, but in reality return value optimization will typically cause this copy to be optimized away.

Answer 5

Just for the record, here's how this program might look like in proper C++:

class Object
{
    int id;

    // private constructor, not for general use
    explicit Object(int i) : id(i) { }

public:
    static Object createWithID(int id)
    {
        return Object(id);
    }
};

int main()
{
    Object obj1 = Object::createWithID(1);
    auto obj2 = Object::createWithID(2);   // DRY

    // return 0 is implied
}

This is probably not what people would generally call a "factory", since factories typically involve some dynamic type selection. The term "named constructor" is sometimes used, though, to refer to the static member function that returns an instance of the class.

Answer 6

By calling Object *obj = new Object(); you do allocate memory on the heap. In the lines following that statement you do return the reference to that object. So far, so good, but you do never delete the object you created to actually free the memory. By calling the function several times you will run in a memory leak.

There are two possible workarounds:

1. static Object createWithID(int id); would return a copy of the Object you create, so it would be enough to allocate it on the stack using

   Object tmp;
   tmp.id = id;
   

2. use c++11 smart pointer to let them handle the memory.

   #include <memory>
   static std::unique_ptr<Object> createWithID(int id)
   {
       std::unique_ptr<Object> tmp(new Object());
       tmp->id = id;
       return std::move(tmp);
   }