Jump to content
  • Advertisement
Sign in to follow this  
Deliverance

Object Factory & cloning products

This topic is 3990 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
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.

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
Sign in to follow this  

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

We are the game development community.

Whether you are an indie, hobbyist, AAA developer, or just trying to learn, GameDev.net is the place for you to learn, share, and connect with the games industry. Learn more About Us or sign up!

Sign me up!