Skip to main content
gamedev.net
🔒 Locked

Making Singleton pattern clearer.

Started by ANSI2000 Dec 26, 2001 at 11:18 AM 29 replies 2k views
Original Post
ANSI2000
ANSI2000
Ok, I understand the Singleton pattern when it should be used and how to implement it, but am not to sure ho wto implement it within the rest of my application and classes. Let''s say I have a class called Logs. This class will write to file for loging purposes. 
  
class Logs
{
   public:
      static Logs getInstance(); // If Logs is NULL, create new instance and return it, if not return already created instance.


      void init(char *pLogsLocation, char *pFileName); // Set the location and file name where the logs should be set.


      void writeLog(char *pMessage); // Write a message to the log


      ~Logs() // Destructor


   private:
      Logs(); // Constructor


      static Logs myLogs;
      char logsLocation[128], fileName[128]; 
}
Do init(), logsLocation[128], fileName[128] have to be static? Also how would I instanciate, intilaize the logs class to be used within my application and other classes. Woudl I have to declare and Logs object per class and instatiate each one by using getInstance? And will I have to call init() over and over again, am asuming init has to be called only once... 
  

void main()
{
    Logs myLogs = Logs::getInstance()
    myLogs.init("c:\\logs", "logfile.txt");

    MyClass myClass;

    myClass.init();
}



class MyClass
{
   public:
      init()
      {
         myLogs = Logs::getInstance();
         myLogs.init("bla", "bla"); // Should this be done?

      }

   private:
      Logs myLogs;
}
WayfarerX
WayfarerX
This is how I do a singleton:

    
class Logs
{
public:
  static Logs& getInstance ()
  {
    assert(singleton != 0);
    return *singleton;
  }
  Logs ()
  {
    assert(singleton == 0);
    singleton = this;
  }
  ~Logs ()
  {
    singleton = 0;
  }
  void init (char *pLogsLocation, char *pFileName);
  void writeLog (char *pMessage);
  void destroy();
private:
  static Logs* singleton;
};
// cpp file

Logs* Logs::singleton = 0;


Then, I create & initialize the singleton before entering the program loop

  
int main ()
{
  Logs logs();
  logs.init("c:\\logs", "logfile.txt");
  while (programRunning)
  {
    // ...Do cool stuff...

    someFunc();
    // ...Do more cool stuff...

  }
  logs.destroy();
  return 0;
}
//

void someFunc ()
{
  Logs::getInstance().writeLog("In someFunc()");
}


I like doing it this way because it clears up who actually owns the singleton, when it gets created, initialized, and destroyed. You create it, initialize it, and from then on use Logs::getInstance() like it was a global object (you can even #define g_Logs Logs::getInstance() and use that). Nothing besides Logs::singleton & Logs::getInstance() should be static.

Just remember that you're going to want to return a pointer or a reference to your singleton from getInstance(), otherwise you will actually copy the singleton when you return it, which doesn't make it very single.

FYI: Game Programming Gems II has a good chapter on this and how to implement automatic singletons with a template base class.

"So crucify the ego, before it's far too late. To leave behind this place so negative and blind and cynical, and you will come to find that we are all one mind. Capable of all that's imagined and all conceivable."
- Tool
------------------------------


Edited by - wayfarerx on December 26, 2001 1:13:32 PM
"There is no reason good should not triumph at least as often as evil. The triumph of anything is a matter of organization. If there are such things as angels, I hope that they're organized along the lines of the mafia." -Kurt Vonnegut
Vetinari
Vetinari
Right, so a better way is to make the constructor private, ensuring compile-time error over run-time error. The instance is static in the getInstance(), ensuring lazy initialization, and no new/deletes. 
      
class Logs
{
public:
  static Logs& getInstance ()
  {
    static Logs instance;
    return instance;
  }
  void init (char *pLogsLocation, char *pFileName);
  void writeLog (char *pMessage);
  void destroy();
private:
  Logs () { }
  ~Logs () { }
};



Mike
"Unintentional death of one civilian by the US is a tragedy; intentional slaughter of a million by Saddam - a statistic." - Unknown
ANSI2000
ANSI2000
The way I know it is like this....

    
class Logs
{
public:
   static Logs &getInstanc()
   {
      if(myLogs == NULL)
         myLogs = new Logs();

      return(myLogs);
   }

   void init(char *, char *);
   void writeLog(char *);

   /* Is this ok?*/
   void destroy()
   {
      if(myLogs != NULL)
         delete myLogs;
   }

private:
   Logs();
   ~Logs();

   static Logs *myLogs;
}


Am not tu sure about destroy though....

Edited by - ANSI2000 on December 26, 2001 6:36:06 PM
Shannon Barber
Shannon Barber
If you make the [dtor] private, then you need to have the sigleton suicide on a reference count release (otherwise, no one can destroy it).

Instead of Init and destroy, try something like GetInstance and ReleaseInstance.

[meant destructor not constructor]

Edited by - Magmai Kai Holmlor on December 28, 2001 10:54:27 AM
The trade-off between price and quality does not exist in Japan. Rather, the idea that high quality brings on cost reduction is widely accepted.-- Tajima & Matsubara
ANSI2000
ANSI2000
I thought that for the singleton pattern the constructor must be private. If not anyone can make an instance. You sure you wherent talking about the destructor?

Houdini
Houdini
One thing you can do to make your design clearer, is to move the Singleton code out of the class and put it in a template. Most implementations look like so (minus the reference counting Magmai suggested, for clarity sake):

      
template<class ClassType>
class Singleton
{
public:
	static ClassType &GetInstance()
	{
		if (!m_instance)
			m_instance = new ClassType;

		return *m_instance;
	}

protected:
	static ClassType *m_instance;
};

template<class ClassType>
ClassType *Singleton<ClassType>::m_instance = NULL;
  
You'd then just derive your new class from this Singleton class like so:

      
class Log : public Singleton<Log>
{
protected:
	Log() {}
	~Log() {}
public:
	// Insert interface here

}


This solves the problem of having to retype all that same code for every class you want to be a Singleton. However, this class suffers from the same thing the original implementation does, that you can't derive a new class from Log because you can't re-implement the static GetInstance function to return your new derived class.

However, if you make your Log class a friend of the Singleton rather than deriving from Singleton, you keep all the previous advantages and gain the ability to inherit since your Log class will no longer has static functions. The only change from the previous example would be to the Log class itself:

  
class LogSingleton
{
	friend Singleton<Log>;

protected:
	LogSingleton() {}
	~LogSingleton() {}
public:
	// Insert interface here

}
  
You can typedef your class like so:

      
typedef Singleton<LogSingleton> Log;


As far as the reference counting that Magmai suggested, I'm not so sure about. If everytime you want to log something you must call GetInterface then ReleaseInterface. This may not be a large issue in some places, but if you use a singleton in a tight loop it will kill performance. You'd have to be aware of this and code around it by getting a pointer to the object before the loop, use it, then release it after the loop. Too much hassle, imo. Also, every time your class was destroyed you'd lose any state information the class previously contained.

I personally create a Destroy function in my Singleton class and specifically call Destroy() on all singleton objects just before the exit of my program. This way everything gets deleted in the order I want and I don't lose state information.

It's not a perfect solution, but the best I can come up with.


- Houdini

Edited by - Houdini on December 26, 2001 12:03:59 AM

[edit: the board garbles multiple source tags on edits, fixed them]

Edited by - Magmai Kai Holmlor on December 28, 2001 6:07:50 AM
Houdini
ANSI2000
ANSI2000
You dont really need to derive a singleton class anyways.....

I still havent gotten an answer why the constructor must be public, every article I have read, says to keep the constructor, protected or private. The destructor should remain public so you can delete all references.

So if we have...
  
class Logs
{
public:
   Logs &getInstance();
   void init(char *, char *);
   void writeToLog(char *);
   void destroy()
   {
      delete instance;
   }
   ~Logs();

private:
   Logs();
   static Logs *instance;
}

class ClassA
{
   /*
   In some method...
   Logs myLog = Logs::getInstance();
   */
}

class ClassB
{
   /*
   In some method...
   Logs myLog = Logs::getInstance();
   */
}

void main()
{
   Logs logs = Logs::getInstance();
   logs.init("c:\temo\", "myLog.txt");

   /* Instanciate and create classes A & B */
   /* Call a few methods of classes A & B */

   /* 
   Shoudl this not free up all references and pointers?
   Or will this cause dangling references or pointers?
   */
   logs.destroy();

}


SteveC
SteveC
The ctor and dtor of singleton''s are normally made private.

The reason is this - there is one instance and you must control access to the construction and destruction of that one object.

consider the following -

  

class Singleton
{
public:

static Singleton* Instance();
static void Destroy();


Singleton();
~Singleton();

private:
static Singleton* siInstance;
};

// someone can then do the following:


delete Instance();


but the siInstance will now be pointing to a dead object. asking for disaster

consider that even if it was ''static Singleton& Instance()''

someone could write ''delete &(Instance())''

evil, yes, but making the dtor private will give a compile time error.

A compile time error is always easier to fix than a run time error. ( well, usually )

an alternative for cleanup issues is to use a simple template which calls a static Destroy method

eg

  

template <class SINGLETON>
class SingletonDestroyer
{
public:
SingletonDestroyer() { }
~SingletonDestroyer() 
  {
  SINGLETON::Destroy();
  }
};

// then put an instance of one at file scope of the singleton

// eg.


SingletonDestroyer<MySingleton> destroyIt;


That template requires the api of the singleton to have a static method Destroy()

this then ensures that the Destroy() method gets called at app exit.

Beware that the order of static destruction ( the destroyIt instance ) of objects is not guaranteed. I have seen it behave flakey when doing abnormal application exit with DLLs

But most of the time this technique works without a hitch.


Houdini
Houdini
quote:
Original post by ANSI2000
You dont really need to derive a singleton class anyways.....


How can you assume that no future singletons you create will ever be derived from? That's
a mighty big assumption.

Heck, if I was using your library I might not like that your Log class doesn't timestamp events.
If all I have is a .lib and header files then I must derive a new instance to add
timestamping.

quote:
Original post by ANSI2000
I still havent gotten an answer why the constructor must be public, every article I have read,
says to keep the constructor, protected or private.


You should not make the constructor public otherwise anyone can make instances of the class.

quote:
Original post by ANSI2000
The destructor should remain public so you can delete all references.


I disagree. As a general rule, whichever class or module created the object should also
delete it. Make another static function that will delete the pointer, and call that before
your application exits.

Leaving your destructor public
allows someone to delete the pointer to your singleton class, as SteveC stated already.


BTW, forgot about making private constructor and destructor myself in my Singleton template
(it was late and I was tired =). It should look like this:

      
template<class ClassType>
class Singleton
{
private
     Singleton() {}
     ~Singleton() {}

public:
     static ClassType &GetInstance()
     {
          if (!m_instance)
               m_instance = new ClassType;

          return *m_instance;
     }

     static void DestroyInstance()
     {
          if (m_instance) {
               delete m_instance;
               m_instance = NULL;
          }

          return *m_instance;
     }

protected:
     static ClassType *m_instance;
};

template<class ClassType>ClassType *Singleton<ClassType>::m_instance = NULL;



- Houdini

Edited by - Houdini on December 27, 2001 6:43:30 PM
Houdini
Void
Void
Take a look at the singleton holder class from "Modern C++ Design".

It explains very clearly what a singleton holder class should provide, and the designs rationale behind it.

Personally, I have a singleton holder class in the files section at groups.yahoo.com/group/boost/files which is inspired by MC++D. It''s called boost_singleton.zip.

..
ANSI2000
ANSI2000
Does any one have a non template sample of the sigleton patern and the way they would use it?

Thanks.
Shannon Barber
Shannon Barber
Opps, yes I meant destructor above

    
class CSingleton
	{
	static CSingleton* ms_This;
	static int ms_cRef;
	CSingleton()
		{}
	~CSingleton()
		{}
	public:
		static CSingleton* GetInstance()
			{
			if(!ms_This)
				ms_This = new CSingleton;
			++ms_cRef;
			return ms_This;
			}

		int ReleaseInstance()
			{
			int c = --ms_cRef;
			if(!ms_cRef)
				{
				delete ms_This;
				ms_This=0;
				}
			return c;
			};

	};

//in .cpp

CSingleton* ms_This=0;
int ms_cRef=0;

//

To use:
CSingleton* pLogger = CSingleton::GetInstance();
//use pLogger

pLogger->ReleaseInstance();

You must call GetInstance to retrieve the singleton pointer, and must have a pointer to call ReleaseInstance.


P.S. If you use a template version, you would set the return type of GetInstance to the templated parameter.

If you really want to be safe:
  
CSingleton* operator =(CSingleton* lhs, CSingleton* rhs)
{
assertMsg(false, "Call GetInstance - Do NOT copy singleton pointers");
}


Magmai Kai Holmlor

"Oh, like you've never written buggy code" - Lee

"What I see is a system that _could do anything - but currently does nothing !" - Anonymous CEO

Edited by - Magmai Kai Holmlor on December 28, 2001 6:30:14 AM
The trade-off between price and quality does not exist in Japan. Rather, the idea that high quality brings on cost reduction is widely accepted.-- Tajima & Matsubara
ANSI2000
ANSI2000
Magmai, you went and made the constructor public in your last example, why? Every body heare says to make it private.....

I dont see why your counting number of references... If the constructor is private and you call getInstance more then once then the static instance should never get initialized more than once anyways, no???

Somebody, has to have a nice clean non templetized sample to share...
Shannon Barber
Shannon Barber
um... that constructor is private.
It''s reference counted so you know when it''s safe to delete it.
You can instantiate a global instance if you don''t want to do anything like that.

pentium3id:
It''s just the idea that only one instance of a class should ever exist, and anything that uses that class ought to use the same instance.
The trade-off between price and quality does not exist in Japan. Rather, the idea that high quality brings on cost reduction is widely accepted.-- Tajima & Matsubara
Arild Fines
Arild Fines
Please tell me that it is the forum thats to blame for the indentation in Magmai's code...

EDIT: Never mind - apparently Opera does some weird things with some of the source tags.

Fantastic doctrines (like Christianity or Islam or Marxism or Microsoft-bashing) require unanimity of belief. One dissenter casts doubt on the creed of millions. Thus the fear and hate; thus the torture chamber, the iron stake, the gallows, the labor camp, the psychiatric ward - Edward Abbey

Edited by - Arild Fines on December 28, 2001 10:44:02 AM
--AnkhSVN - A Visual Studio .NET Addin for the Subversion version control system.[Project site] [IRC channel] [Blog]
ANSI2000
ANSI2000
Here is a test I created...

  
#ifndef SINGLETON_H
#define SINGLETON_H

#include <stdio.h>
#include <iostream>

class Logs
{
public:

	static Logs *getInstance()
	{
		if(instance == 0)
			instance = new Logs;

		return(instance);
	}

	void init(const char *pLocation, const char *pFileName)
	{
		strcpy(location, pLocation);
		strcpy(fileName, pFileName);
	}

	void writeToFile(const char *pMessage)
	{
		std::cout << "Write to file: " << location << fileName << " Message: " << pMessage << "\n";
	}

	void release()
	{
		delete instance;
		instance = 0;
	}

	~Logs() {};

private:
	Logs() {};

	static Logs *instance;

	char location[128];
	char fileName[128];

};

#endif

//////////////////////////////////////////////////////////////////

#include "singleton.h"

Logs *Logs::instance = 0;

//////////////////////////////////////////////////////////////////


#include "singleton.h"

void main()
{
	Logs *myLogs1 = Logs::getInstance();
	myLogs1->init("c:\\temp\\", "log.txt");
	myLogs1->writeToFile("Hello1");

	Logs *myLogs2 = Logs::getInstance();
	myLogs2->writeToFile("Hello2");

	//myLogs2->release();

	delete myLogs2;

	myLogs1->writeToFile("Hello1");
	myLogs2->writeToFile("Hello2");


	Logs *myLogs3 = Logs::getInstance();
	myLogs3->writeToFile("Hello3");
}


delete and release seem to do the same thign... Any call to writeToFile after the delete or realese cout will print garbage...
Shannon Barber
Shannon Barber
quote:
Original post by ANSI2000
delete and release seem to do the same thign... Any call to writeToFile after the delete or realese cout will print garbage...


Which is why you need to reference count So it''s only deleted if nobody is still using it, yet you consistently call Release after every GetInstance.

If it''s a singleton, you don''t need an init function - or at least it shouldn''t be a public method. Make it private and call it when the object is created (new''ed) in the GetInstance method.

...or make a global instance.
The trade-off between price and quality does not exist in Japan. Rather, the idea that high quality brings on cost reduction is widely accepted.-- Tajima & Matsubara
Dire.Wolf
Dire.Wolf
A huge problem with reference counting is that you are constantly initializing and destroying the singleton. If the creation or destruction of the Singleton is expensive, reference counting does not provide a very elegant solution.

void DoSomething()
{

    // 1st call, creates singleton + increments reference count
   &nbspSingleton* p = Singleton::Instance();

    // do something with p

    // destroys singleton because reference count becomes 0.
    // p->Release();

}

int main()
{
   &nbspDoSomething(); // singleton created/destroyed
   &nbspDoSomething(); // singleton created/destroyed
   &nbspreturn 0;
}

Just a thought.





Dire Wolf
www.digitalfiends.com
[email=direwolf@digitalfiends.com]Dire Wolf[/email]
www.digitalfiends.com

Topic Locked

This topic has been locked by a moderator. New replies are not allowed.

Sign in to reply to this topic.