Sign in to follow this  

Object Factory & cloning products

This topic is 3777 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

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]

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
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);
};

Share this post


Link to post
Share on other sites
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;

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
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]

Share this post


Link to post
Share on other sites
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;
}

Share this post


Link to post
Share on other sites
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?

Share this post


Link to post
Share on other sites
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;
}
};

Share this post


Link to post
Share on other sites
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).

Share this post


Link to post
Share on other sites

This topic is 3777 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this