Sign in to follow this  
Googol PL3X

Singleton Design pattern troubles

Recommended Posts

In my program, there is an _Engine object, and I am using the Singleton design pattern to create a single object of it. In terms of my problem, there are 3 main files that should be known, Engine.h - contains the class definition for Engine as follows:
class _Engine
{
public:

	void UpdateTime();
	void UpdateEnemies();
	Image *GetEnemyImage() const;
	
	static _Engine* Instance();
	
private:

	float oldTime, deltaMilliSeconds;
	vector<Enemy*> Enemy_Vector;
	vector<Enemy*>::const_iterator iter;
	Image *_enemyImage;
	
	static _Engine *pinstance;
	
protected:

	_Engine();
    _Engine(const _Engine&);
    _Engine& operator = (const _Engine&);
	~_Engine();

};


Simple enough, most of it isn't related to the problem, but source always helps. And so next I have Engine.cpp which contains the definition for _Engine's functions and is as follows:
//---------- CONSTRUCTOR -----------//
_Engine::_Engine() : oldTime(ticks_to_millisecs(gettime())), Enemy_Vector(vector<Enemy*>()), _enemyImage(new wsp::Image)
{ 

	srand(time(0));
	_enemyImage->LoadImage("enemy.png");
	deltaMilliSeconds = 0;

}

//---------- DESTRUCTOR -----------//
_Engine::~_Engine()
{

	Enemy_Vector.clear();
	
}

//---------- UPDATE TIME -----------//
void _Engine::UpdateTime()
{
	float newTime = ticks_to_millisecs(gettime());
	deltaMilliSeconds = newTime - oldTime;
	
	oldTime = newTime;
}

//---------- DRAW ENEMIEs -----------//
void _Engine::UpdateEnemies()
{
	if (Enemy_Vector.empty())
	{
		for (iter = Enemy_Vector.begin(); iter != Enemy_Vector.end(); ++ iter)
		{
			(*iter)->Update(deltaMilliSeconds);
		}
	}
}

//---------- GET ENEMY IMAGE -----------//
inline wsp::Image *_Engine::GetEnemyImage() const
{

	return _enemyImage;
}

//---------- Initialize Pointer ----------//
_Engine *_Engine::pinstance = 0;

//---------- INSTANCE -----------//
_Engine *_Engine::Instance()
{
	if (pinstance == 0)
	{
		pinstance = new _Engine;
	}
	 
	return pinstance;
}

The function most interesting above is *_engine::Instance(), basically it will cehck if pinstance has no address, if it has no address, it will create a new _Engine object and return it. The other file file is Enemy.cpp, I won't post the source as their is little point at this stage. But the main problem is that I need Enemy.cpp to be able to see _Engine *Engine = _Engine::Instance(); the Engine part so I can call it's functions, I tried making it extern in both Engine.h and Engine.cpp and including bother respectively in Enemy.cpp, but I just can't get it to work. If someone can help, I would greatly appreciate it.

Share this post


Link to post
Share on other sites
I'd rather create the object in one place and have it accessed globaly rather than creating a new pointer to an Engine object (if that's what you mean?). Would that even make a differnece?

edit: I added _Engine *Engine = _Engine::Instance(); after #include "Engine.h" in Enemy.cpp and it worked (as expected), but would I have to do this in every file I want to use it? And would this defeat the purpose of having it singelton and would they all just point to the same object if I did it that way?

Share this post


Link to post
Share on other sites
Quote:
Original post by Googol PL3X
edit: I added _Engine *Engine = _Engine::Instance(); after #include "Engine.h" in Enemy.cpp and it worked (as expected), but would I have to do this in every file I want to use it? And would this defeat the purpose of having it singelton and would they all just point to the same object if I did it that way?
Yup, that's what you need to do to use it. The purpose of a singleton is that no matter how many times you call _Engine::instance(), or what files you call it from, you get the same object back.

Share this post


Link to post
Share on other sites
i'm sure plenty of people will be along shortly to point out why using a singleton is a really awful idea. [grin]

Another point is that the name "_Engine" is *really* bad.
Names beginning with an underscore followed by a capital letter, or with double underscore are reserved by the implementation. The standard library implementers are allowed to define a their own global variables, macros, functions or anything else named _Engine, which will lead to lots of fun errors in your code. Avoid leading underscores in your own names. :)

Share this post


Link to post
Share on other sites
1) Do a quick search on the forums for "Singleton". Many people (with good reason) think they are best avoided.

2) As has been stated in another of your threads, you are using illegal identifiers.

3) There is no need to explicitly initialise the enemy vector, it will initialise itself just fine.

4) std::vectors can clean up after themselves. Your destructor, as is, does nothing it wouldn't do if you left the compiler generate it (this is the preferred way to do it). However, you do allocate memory in the constructor (and presumably during runtime too, with the vector) that you do not deallocate. Remember, a vector only manages the memory it requests. If you put pointers to objects dynamically allocated into a vector, you *still* need to call delete on them.

5) Creating a "new pointer" isn't a big deal. In fact, this is how I would use your class:

void foo()
{
// the dereference would not be necessary if instance() returned a reference
Engine &engine = *Engine::Instance();

engine.frobnicate();
engine.discombobulate();
// etc
}


Share this post


Link to post
Share on other sites
Quote:
Original post by Googol PL3X
Hey rip-off, thanks for the advise, I've changed pretty well most of it. Could you further elaborate 2 and 5 of your post? Thanks


#2 is just what I said, avoid leading underscores.

Not sure what you want to know about #5. seems pretty clear to me. return a reference instead of a pointer, and just call Instance() when you need to use the singleton.

Share this post


Link to post
Share on other sites
Quote:
Original post by Googol PL3X
I've not done design patterns and such before tonight

So why exactly do you think you should use a Singleton?

Share this post


Link to post
Share on other sites
Quote:
Original post by DevFred
Quote:
Original post by Googol PL3X
I've not done design patterns and such before tonight

So why exactly do you think you should use a Singleton?
There is nothing wrong with creating multiple instances of this class - this alone automatically rules out the need of a singleton but not the need of a global.
Whether or not the rest of the system requires global access to an instance of it depends on how well the rest of the system has been designed - if it is all well thought out then global access will not be needed either.

Share this post


Link to post
Share on other sites
There is a huge block of text somewhere that has a long winded explenation of why not to use singletons. But Id like to throw my short version at you.

OK, before my version, Id just like to add another point. If this code is actually being used so you can get an idea of how singletons work, by all means keep it up, it might not ever come in handy, but its good to know. Ok, on with the point.

Unless you are working on code for a device with very limited memory (anything more then a hundred or so is likely not limited) where having more then one instance of a class would cause problems. Unless you need that kind of efficiently, just make a regular class and only use it once, it makes things alot easier down the road.

Share this post


Link to post
Share on other sites
I was creating a small game to play around with some of the design patterns, and try and learn how to use them.

I have read the article provided by phantom and decided I will try to avoid them in future, although just a bit of practice isn't going to hurt. Rather than doing this, I will probably just create an instance of Engine at the bottom of the class definition in Engine.h and make it extern and just include it where need be.

Share this post


Link to post
Share on other sites

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