Object Factory & cloning products

Started by
11 comments, last by Deliverance 16 years, 8 months ago
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]
Advertisement
What IDE are you using?

When I tried to compile your code using VC++ express, I got a bunch of errors on this line:

CGeometricObjectFactory::GetInstance().RegisterProduct(					SPHERE_ID, CreateSphere);


Changing it to -

bool b = CGeometricObjectFactory::GetInstance().RegisterProduct(					SPHERE_ID, CreateSphere);


fixed them.

Then I got a linker error saying there was an unresolved external reference to instance. putting

template <class T>CObjectFactory<T> CObjectFactory<T>::instance;


right after the class declaration fixed that and now I don't get any errors.

BTW, in your GetInstance() function, you're returning a copy of the factory. You should modify it to return a reference.
Quote:
CGeometricObjectFactory::GetInstance().RegisterProduct(SPHERE_ID, CreateSphere);

This code is 100% illegal, because it's not in any executable block. You have to call that method from within some function, such as main(), or abuse initialization of global variables to do it.

You also don't provide storage for the static variable.

Other minor issues: use <cstdio> in C++, not <stdio.h>, don't employ "using namespace std" in a header file, don't return by value from GetInstance(), consider using a template parameter for the key type rather than hard-coding int (you can templatize the producer too if you want to get really generic), throw std::exception-derived classes, not const char*.

You can (and should) implement factories without singletons. You'll have significantly more robust code as a result.
Thanks for the hints! I made those modifications and now I'm returning a reference to the class but it still does not work, I'm getting a bunch of warnings and the following error:

I'm using microsoft visual studio 6.0.
"c:\microsoft visual studio 6\vc98\include\xtree(103) : error C2040: 'protected: static class CObjectFactory<class CGeometricObject> * CObjectFactory<class CGeometricObject>::instance' : 'class CObjectFactory<class CGeometricObject>' differs in level
s of indirection from 'class CObjectFactory<class CGeometricObject> *'"

The code looks like this:
#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);	~CObjectFactory();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(){	delete instance;	instance = NULL;}template <class T>CObjectFactory<T>& CObjectFactory<T>::GetInstance(){	if (!instance)			instance = new CObjectFactory<T>;		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());}template <class T>CObjectFactory<T> CObjectFactory<T>::instance;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;	}	bool b = CGeometricObjectFactory::GetInstance().RegisterProduct(					SPHERE_ID, CreateSphere);};
Quote:Original post by Deliverance
I'm using microsoft visual studio 6.0.

Its out of date and broken for a start.

But, you've changed instance to a pointer and forgot to reflect this change in the static instantiation outside of the class:

template <class T>
CObjectFactory<T>* CObjectFactory<T>::instance;
Quote:
I'm using microsoft visual studio 6.0.

Then stop. Visual C++ Express (version 8!) is free, and actually compiles C++. VC++6 is a nasty, ancient hack of a "C++" compiler that was around before the language was standardized. I strongly urge you to upgrade as soon as possible.

Quote:
"c:\microsoft visual studio 6\vc98\include\xtree(103) : error C2040: 'protected: static class CObjectFactory<class CGeometricObject> * CObjectFactory<class CGeometricObject>::instance' : 'class CObjectFactory<class CGeometricObject>' differs in level
s of indirection from 'class CObjectFactory<class CGeometricObject> *'"

Within the class definition for CObjectFactory<T>, you declare instance as a CObjectFactory<T>* -- but when you actually declare storage for the variable, outside the class, you omit the *, declaring instance as a plain CObjectFactory<T>. Add the missing *.

Something else I forgot to mention is that you should consider abandoning the practice of prefixing classes with "C" as it doesn't provide a whole lot of useful information -- the type is obviously not a primitive built-in type, and it doesn't matter if the object is a class or struct since they're the same thing.
Thanks for everything, I'm on my way getting Visual C++ Express!
Good! [smile]
Just a heads up, you may find you have a minor code maintenance task ahead of you. Code that compiled fine on the archaic VC++6 compiler won't necessarily work in VC++8 quite simply because the old compiler would allow invalid C++ but the modern one will return an error and force you to fix it.
Just don't be tempted to crawl back to VC++6 to avoid fixing faulty code [wink]
I have to say I'm quite a beginner when it comes to design patterns so please excuse my noob questions. I have tried to implement a cloning facility in the factory like this:
template <class T>T* ObjectFactory<T>::CloneProduct(T* product){		T* clonedProduct = GetProduct(product->GetType());	*clonedProduct   = *product;	return clonedProduct;}

but it does not work. The line *clonedProduct = *product; is not doing what it's supposed to do. Why is that?
Here's the full code:
#include <cstdio>#include <iostream>#include <map>using namespace std; //class ErrorClass;template<class Product>class ObjectFactory{public:		// Pointer to function that will create an object	typedef Product* (*CreateProductFunction)();		// This will clone a given product	Product* CloneProduct(Product* product);	// ObjectFactory is a singleton, returns instance	static ObjectFactory& 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:	ObjectFactory(){}	ObjectFactory(const ObjectFactory&){}	const ObjectFactory operator=(const ObjectFactory&){}		static ObjectFactory *instance;	typedef map<int, CreateProductFunction> CallbackMap; 	CallbackMap productCallbacks;};template <class T>ObjectFactory<T>* ObjectFactory<T>::instance = NULL;template <class T>ObjectFactory<T>& ObjectFactory<T>::GetInstance(){	if (!instance)			instance = new ObjectFactory<T>;	return *instance;}template <class T>T* ObjectFactory<T>::CloneProduct(T* product){		T* clonedProduct = GetProduct(product->GetType());	*clonedProduct   = *product;	return clonedProduct;}template <class T>bool ObjectFactory<T>::RegisterProduct(int productID, CreateProductFunction func){	return productCallbacks.insert(		CallbackMap::value_type(productID, func)).second ;}template <class T>	bool ObjectFactory<T>::UnRegisterProduct(int productID){	return productCallbacks.erase(productID)==1;}template <class T>T* ObjectFactory<T>::GetProduct(int productID){	CallbackMap::const_iterator i = productCallbacks.find(productID);	if (i==productCallbacks.end())	{		//throw ErrorClass;		return NULL;	}	return (i->second)();}class GeometricObject{public:		int GetType(){return type;}	void SetType(int ID){type=ID;}protected:	int type;};	typedef ObjectFactory<GeometricObject> GeometricObjectFactory;class Sphere: public GeometricObject{public:	Sphere():x(0),y(0),radius(0){}	float x,y;	float radius;	const Sphere& operator=(const Sphere& s)	{		x = s.x;		y = s.y;		radius = s.radius;	}	void Set(float x, float y, float radius)	{		this->x = x;		this->y = y;		this->radius = radius;	}	void PrintValues()	{		cout << x << " " << y << " "<< radius << endl;	}};namespace{	const int SPHERE_ID = 1;	const int PLANE_ID  = 2;	GeometricObject *CreateSphere()	{		Sphere *sphere = new Sphere;		sphere->SetType(SPHERE_ID);		return sphere;	}	bool a = GeometricObjectFactory::GetInstance().RegisterProduct(		SPHERE_ID, CreateSphere);};int main(){	Sphere *s1 = (Sphere*)GeometricObjectFactory::GetInstance().GetProduct(1);	s1->Set(1,2,3);	Sphere *s2 = (Sphere*)GeometricObjectFactory::GetInstance().CloneProduct(s1);	s1->PrintValues();	s2->PrintValues();	delete s1;	delete s2;	return 0;}
Quote:but it does not work. The line *clonedProduct = *product; is not doing what it's supposed to do. Why is that?

Erm, actually its doing exactly what its 'supposed' to do, granted perhaps not what you 'wanted' it to do.

What did you want it to do compared with what actually happens?

This topic is closed to new replies.

Advertisement