Back to General and Gameplay Programming

Object Factory & cloning products

Adrian Manolache · 2007-08-15T00:06:53

I'm trying to implement the object factory pattern but I'm having some dificulties, I get a strange error that says that the GetInstance function is being defined multiple times. Here's the code: #include <stdio.h> #include <map> using namespace std; class CGeometricObject { public: CGeometricObject(){}; void SetType(int id){type = id;} int GetType(){return type;} private: int type; }; class CSphere:public CGeometricObject { }; class CPlane:public CGeometricObject { }; template<class Product> class CObjectFactory { public: // Pointer to function that will create an object typedef Product* (*CreateProductFunction)(); // ObjectFactory is a singleton, returns instance static CObjectFactory GetInstance(); // It will return true upon successuful registration bool RegisterProduct(int productID, CreateProductFunction func); // It will return false upon successuful unregistration bool UnRegisterProduct(int productID); // Will return the specified product Product* GetProduct(int productID); protected: CObjectFactory(){} CObjectFactory(const CObjectFactory&){} const CObjectFactory operator=(const CObjectFactory&){} static CObjectFactory instance; typedef map<int, CreateProductFunction> CallbackMap; CallbackMap productCallbacks; }; template <class T> CObjectFactory<T> CObjectFactory<T>::GetInstance() { return instance; } template <class T> bool CObjectFactory<T>::RegisterProduct(int productID, CreateProductFunction func) { return productCallbacks.insert( CallbackMap::value_type(productID, func)).second ; } template <class T> bool CObjectFactory<T>::UnRegisterProduct(int productID) { return productCallbacks.erase(productID)==1; } template <class T> T* CObjectFactory<T>::GetProduct(int productID) { CallbackMap::const_iterator i = productCallbacks.find(productID); if (i==productCallbacks.end()) { throw "Error"; return NULL; } return (i.second()); } typedef CObjectFactory<CGeometricObject> CGeometricObjectFactory; namespace { const int SPHERE_ID = 1; const int PLANE_ID = 2; CGeometricObject* CreateSphere() { CSphere *sphere = new CSphere; sphere->SetType(SPHERE_ID); return sphere; } CGeometricObject* CreatePlane() { CPlane *plane = new CPlane; plane->SetType(PLANE_ID); return plane; } CGeometricObjectFactory::GetInstance().RegisterProduct( SPHERE_ID, CreateSphere); }; int main() { return 0; } [Edited by - Deliverance on August 14, 2007 2:05:46 PM]

General and Gameplay Programming Programming

Started by Deliverance August 14, 2007 07:14 AM

11 comments, last by Deliverance 16 years, 8 months ago

jpetrie

13,220

August 14, 2007 02:34 PM

Quote:
Secondly, count the number of times you call new and count the number of times you call delete, they should always be equal, no exceptions, ever.

Lies! You should never call delete (that is to say, use smart pointers and other RAII objects that handle this muckery for you, they're better than you when it comes to this obnoxious tedium).

Quote:
The line *clonedProduct = *product; is not doing what it's supposed to do. Why is that?

You are doing static dispatch on the base type's overloaded operator; this is mangling your objects. The typical way the clone idiom is implemented involves calling a virtual clone method on the object, not the factory. In other words, the base class should have a pure-virtual clone() method implemented by all derived classes. The clone() method allocates a new derived type, initializes it based on the state of the invoking object, and returns the new object.

Your factory clone method can simply defer to this object clone method, like

template <class T>T* ObjectFactory<T>::CloneProduct(T* product){	  return product->clone();}

CGeometricObject would have a pure-virtual clone() method (virtual CGeometricObject* clone() const = 0;), and each child would implement it, perhaps, in the case of CSphere, as

class CSphere : public CGeometricObject{  public:    CSphere* clone() const // "covariant return types" allow the return type to be CSphere    {      CSphere *result = new CSphere(this->radius,this->position); // assuming CSphere has these...      return result;    }};

dmatter

4,872

August 14, 2007 02:40 PM

Quote:Original post by jpetrie
Quote:
Secondly, count the number of times you call new and count the number of times you call delete, they should always be equal, no exceptions, ever.

Lies. You should never call delete (that is to say, use smart pointers and other RAII objects that handle this muckery for you, they're better than you when it comes to this obnoxious tedium).

Appologies, I edited my post after noticing they were in-fact equal as I'd misread the clone function [rolleyes]
But yes, use RAII forever and always.

Anyway, as for the original problem, either go with the above design (better), or similarly you could make the operator= a virtual method within geometric object (same idea, but not as clear).

David Gill :: GitHub :: twitter .