Correct way to implement Singleton

cocos2d-x C++
#1

Currently I’m using this approach:

class Singleton {
public:
  static Singleton &getInstance()   {
    static Singleton *instance = new Singleton();
    return *instance;
  }
void getData();

private:
  Singleton() {}
};

In this way I can use a method from Singleton writing:

Singleton::getInstance.getData();

And this seems the right way reading a lot of tutorials for C++11.
But reading through cocos Director singleton code (also FileUtils etc…), I have seen that Cocos uses this other approach:

class Singleton {
public:
  static Singleton *getInstance()   {
    instance = new Singleton();
    return instance;
  }
void getData();

private:
  Singleton() {}
  static Singleton *instance;
};

With this approach I have to write:

Singleton::getInstance->getData();

Because of the pointer *getInstance instead of reference &getInstance.

I think the difference is big, but I don’t know if one way is correct and the other don’t.

Please help me to sorting out this concept.

1 Like
#2

I use the first way. I don’t even explicitly allocate with new, I just do it like this:

Singleton& Singleton::getInstance() {
    static Singleton singleton;
    return singleton;
}

It’s not a big deal, but I’ve mostly seen it done this way with C++11. I wouldn’t spend any time thinking too hard about this unless you have a specific situation, like the possibility of something getting screwed up because you’re trying to initialize/instantiate your singleton in a multithreaded situation.

1 Like
#3

Singletons are a very controversial topic :smiley:

Personally I would strongly recommend against using a singleton because it’s hard to control when it constructs/destructs, and makes your dependancies obscure.

A good summary of singleton evils can be found here: https://blogs.msdn.microsoft.com/scottdensmore/2004/05/25/why-singletons-are-evil/

A better approach is to use dependancy injection where ever possible

e.g:

class MyClass
{
public:
    void foo() { print("foo"); }
}

void doFoo(MyClass& _myClass)
{
    _myClass->foo();
}

void main()
{
    auto sharedMyClass = new MyClass();

    //inject dependency, cumbersome but transparent and the function can be guaranteed what it needs
    doFoo(*sharedMyClass);
    
    delete sharedMyClass;
}

If you don’t want to go down the dependancy injection route and really want to have global access, which I can understand as it can be time consuming, then I would suggest implementing static providers instead.

template<class T>
class Provider
{
public:
    static void setInstance(T* _instance)
    {
        s_instance = _instance;
    }
    T* getInstance()
    {
        return s_instance;
    }
protected:
    static T* s_instance;
}

void doFoo()
{
    auto myClass = Provider<MyClass>::getInstance();
    assert(myClass); //provider may not be set so we should assert if it is not
    myClass->foo();
}

void main()
{
    //initialise Provider
    auto sharedMyClass = new MyClass();
    Provider<MyClass>::setInstance(sharedMyClass);

    //call code that may require sharedMyClass
    doFoo();

    //cleanup provider
    Provider<MyClass>::setInstance(nullptr);
    delete sharedMyClass;
}

Hope this helps

1 Like
#4

Thank you @qwpeoriu, do you also know why Cocos developer use the other way?

@almax27 really thank you for this explanation.
Also other ppl discouraged me to use singletons, so I’m trying to modifying all my codes trying to implement a global static object, but I think I’m destroying all :stuck_out_tongue:

If I used a global static object I lost the data scope when I call the sharedobj from file A or file B.

Now I’m reading your approach with static provider (totally obscure to me :stuck_out_tongue: ) , the boring part is the setInstance, so I have to set this sharedobj in a prior file, like appdelegate and then use it in all other files I think…

Is there a more simplified approach for a sharing game data object?

#5

I use a single singleton, and frankly ignore the ‘singletons are bad’ cry-babies.

As with any pattern or paradigm, it can be misused, overused etc. - but used properly and sensibly a singleton is a fine pattern.

The argument that ‘globals are bad’ is what drives the ‘singletons are bad’ approach - but are Globals bad per se? Of course not! over-use or mis-use is bad.

Few would say that ‘if’ is bad - but an if nested to 27 levels (yes, I have seen one!) is, if not outright bad, at least not good!!

For example - in my games I have a controller class - it essentially replaces the HelloWorldScene class provided by cocos’ default template - and it controls the game.

So it knows where in the game world we are, that sort of stuff, that many different classes might be interested in.

So - if a child-of-a-child-of-a-child-of-a … parent needs to ask how far it is from the game ‘window’ it has access to the controller (via getInstance()) and can ask.

Furthermore - if a child-of-a… etc. wants to let other children know something has happened (say it’s exploded and wants to let everything on-screen know so they can take damage) it can call
GameController::getInstance()->explosionAt(Vec2 location, float energy);

The GameController can then handle this by ‘passing on’ that information through its collection of game objects, or simply setting some ‘flag’ so that other game objects can query it

if (GameController::getInstance()->isExplosion()) …

essentially this is eventing on the cheap - but it is quick, simple, and easily understandable.

The code I use for my singleton is:

GameController* GameController::getInstance()
{
	if (!iInstance)
	{
		iInstance = new GameController();
		srand(static_cast <unsigned> (time(0))); // initialise random number generation
		

	}

	return iInstance;
}

But I just stole that from somewhere - I’m pretty much a C++ noob.

#6

@Levis I would assume cocos2d-x uses that implementation of the singleton pattern because it’s what the author was familiar with. Cocos2d-x probably predates C++11 (I dunno, just a guess).

@Maxxx I completely agree with you. When I use a singleton it’s because I specifically want a global variable. Tight coupling isn’t necessarily a bad thing if it’s quick, easy, and gets the job done. After all I’m just writing a game that’s a few thousand lines long, not enterprise level software.

#7

Totally agreed.

+1
I am doing the same thing. It makes a lot easier to handle all things.
By that i can have my prev_scene, cur_scene info. Even i can send arguments to prev scene to next scene.
Also i have some platform specific code which i can handle through single API in my whole CPP game.
Also interrupt handling n all the stuffs.
Thing is you should know what you are DOING! :smile:

1 Like
#8

Thank you @Maxxx, so you use the Singleton like in Cocos Api, with pointer * and not with reference &, infact you call methods with -> syntax.
I’m C++ noob too :stuck_out_tongue: and I think that the differences are minimal.
And like @qwpeoriu says in a game it doesn’t seems to be so bad thing :smiley:

Just sometimes I got some problem when I have to access some particular structured data in Singleton like RapidJSON, but I think that in this case it’s better to create some method for getter and setter things and return only the value that I have to use without exposing all singleton datas.

#9

Hey guys,

I want to clarify a doubt.

I’ve created and using my Singleton class in following manner.

Taken reference from Singleton
C++11 way of creating singleton instance.

GameManager.h

#ifndef LIB_CONTROLLER_GAMEMANAGER_H_
#define LIB_CONTROLLER_GAMEMANAGER_H_


class GameManager
{
public:
  static GameManager* getInstance()
  {
    static GameManager *instance = new GameManager();
    return instance;
  }

  // Game Play
  int getGameScore();
  void setGameScore(int latestScore);

private:
  GameManager() {};
  int gameScore = 1;
};

#endif /* LIB_CONTROLLER_GAMEMANAGER_H_ */

GameManager.cpp

#include "cocos2d.h"
#include "../Controller/GameManager.h"

USING_NS_CC;

int GameManager::getGameScore()
{
    CCLog("\n\n\nGETTING GAME SCORE: %d\n\n\n", this->gameScore);
    return this->gameScore;
}

void GameManager::setGameScore(int latestScore)
{
    CCLog("\n\n\nSETTING GAME SCORE: %d\n\n\n", latestScore);
    this->gameScore = latestScore;
}

Usage is like::: I’ve checked it by using in 2 different scenes and working fine.

   GameManager::getInstance()->getGameScore();
   GameManager::getInstance()->setGameScore(50);

1) That’s my entire code. I wanted to know, apart from this all above, what else do you add. I mean, is the above code all what I’ve to write and where I’ve to keep on adding functions, variables and keep on setting, getting them?
(I would be using CC_SYNTHESIZE(.., .., ..) approach for writing getters and setters but that’s not the question)

2) How do you free memory memory when the cocos2d-x app is closed or when it is no longer neeeded? Also, I’ve not added deconstructor. I am not sure if following code must be added and called!

void GameManager::purge()
{
    CC_SAFE_DELETE(this->getInstance());
}

3) Also, do you think, I should be using new (std::nothrow) GameMananger(); instead of new GameMananger(); to create the instnce?

Also, I understand the pros and cons of using Singleton and also that service locator seems to be a safe and good approach. But given the size of my game, I think I should use Singleton for now.

Purpose of my singleton would be simple:
Managing Game score struct, SDKBox stuffs if anything, theme settings, and userDefault for storing and retrieving data.

@Maxxx @smitpatel88 @slackmoehrle
Can someone plz have a look at it.

Thanks :slight_smile:

#10

Don’t use singletons.

If you need global access, use a provider pattern.

http://gameprogrammingpatterns.com/service-locator.html

It really isn’t much more work now, but refactoring later could be a pain.

1 Like
#11

Nothing wrong with singletons at all.
As with any pattern or construct, they have their uses -good and bad.

#12

I don’t have much time to look - but I think your code could be wrong…
IN your getInstance you seem to always create a new instance
Should you not have
if (!instance) {}
return instance;

Sorry I don’t have longer to look @ it.

#13

oops… I’ve edited my post.
I think @Maxxx is right.
I’ don’t have expirience with c++11.

static GameManager* getInstance()
{
  static GameManager instance;
  return &instance;
}
#14

Ahhh - yes. I still wasn’t sure so typed it in and ran it in debug - got two references and lo and behold, they are the same!

Very confusing, this C++ lark!

#15

@almax27 @nikita_cz @Maxxx
Thanks for your reply. I’ve checked it and it worked. I’ve also given reference of link from which I am using code. So there is no doubt that above implementation is wrong. Also, if we look at the 1st post in the thread, it implements similarly.

C++11 way is thread safe hence I’ve used it instead of what @Maxxx, you’re suggesting me. Also, this really doesn’t matter because it’s single threaded in my case.

Plz go through whenever you’ve time. I think incomplete reply won’t help as I’ve clearly mentioned that it’s C++11 way and it works. I just need to ask few extra stuffs which you guys do and I am not sure if I am forgetting.

@almax27
First my question is not regarding whether I should use it or not. But since you wish to tell me about provider class, I’ve some reasons. I’ve already stated in the last that I understand that service provider approach is better but it’s unnecessary for my game. I’ve also given the same link over here…

Do you have concrete reason why I shouldn’t use Singleton. I read it everywhere, people just right away say that it’s bad pattern without really understanding the requirement. But some people say that, Singleton is fine in some cases. I saw provider pattern, it’s doable but it looks highly unnecessary code to write to achieve simple thing. I mean, for example, if you see the link, why is even Audio needed when ConsoleAudio is there. I ALREADY know that I won’t make any other inheritance of the Audio class like LoggerAudio, etc as shown in example. So it doesn’t make sense for me to add Audio and then ConsoleAudio. And why will I’ve to create and service and then extend the service just for small portion of functionality. Hence, my requirements are small and so I’m using Singleton class.

So, even if I have to use Provider pattern, I won’t create interface but directly the class I need. :stuck_out_tongue: :smiley:

My question is still unanswered. Please have a look at the doubts first before putting replies otherwise my original post will get lost amidst all the replies.
Thanks :slight_smile:

#16

Singleton:

#ifndef  _GAMEOBJECT_H_
#define  _GAMEOBJECT_H_

class GameObject
{
    public:
        static GameObject* Instance();
        
    private:
        GameObject();
        GameObject(const GameObject&);
        GameObject& operator= (const GameObject&);
        static GameObject* pinstance;
};

#endif // _GAMEOBJECT_H_

#include "GameObject.h"

GameObject* GameObject::pinstance = 0;

GameObject* GameObject::Instance()
{
	if (pinstance == 0)
	{
		pinstance = new GameObject();
		pinstance->initInstance();
        pinstance->startInstance();
    }
	
	return pinstance;
}

GameObject::GameObject() {}
2 Likes
#17

Thanks slackmoehrl for the reply.

I also looked at stackoverflow that we should add these things as a good practice… So, now I’ve added it.

Do, I’ve to add and make use of desconstructor? Or it’s fine it let it as it is?

Thanks :slight_smile:

#18

I do both.

#19

I am sorry I didn’t understand. You mean you add deconstructor as well? Something Like below! :slight_smile:

GameManager::~GameManager(void )
{
CC_SAFE_RELEASE(...variable 1 in Singleton Class...);
CC_SAFE_RELEASE(...variable 2 in Singleton Class...);
CC_SAFE_RELEASE(...variable 3 in Singleton Class...);
}

@slackmoehrle
If so, then at what part of the game, do you exactly delete instance;?

#20

@catch_up

Making a singleton noncopyable is good practice, but just FYI there’s an updated way to do this since C++11.

Setting a C++ function to delete

Prohibiting Copy/Assignment in a class

Personally I use boost::noncopyable (which uses the = delete under the hood) but I understand if people want to avoid including boost in their codebase.