Singleton Design pattern troubles

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

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));
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 on other sites
Surely you just need to #include "Engine.h" and then use _Engine *Engine = _Engine::Instance(); wherever you want?

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 on other sites
A global pointer to a singleton? Now THAT's something new :))

Share on other sites
Hah yeah, I see, I've done some more research and sorted out most of the problems.

Share on other sites
Quote:
 Original post by Googol PL3Xedit: 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 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 on other sites
Ahh, thanks heaps guys (Evil Steve), I've not done design patterns and such before tonight, so it helps to have a bunch of professional programmers around :)

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 on other sites
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

1. 1
2. 2
3. 3
Rutin
19
4. 4
5. 5

• 14
• 30
• 13
• 11
• 11
• Forum Statistics

• Total Topics
631780
• Total Posts
3002314
×