In the interest of creating an inheritable singleton class which protects against as many programmer errors as possible, I am submitting my current singleton class (based on the one found in Enginuity) to the GameDev community in the hope that it will be properly critiqued. My goal is to have a base inheritable singleton class with the least setup effort (only calls to Create()/Destroy() needed, preferably less), and yet the most error-checking (such as notifying the programmer when two instances are created). So basically, I would appreciate it if you could look over the class, point out things you would do differently, and mention any particular reason for doing so.

template< class T > class Singleton {
protected:
	Singleton() {
		// check if instance already exists

		if( SingletonPtr == 0 )
			SingletonPtr = static_cast<T*>( this );
	}
	// unimplemented copy constructor

	Singleton( Singleton<T> const& );
	// destructor (only called by Destroy)

	~Singleton() {}
public:
	// unimplemented assignment operator

	Singleton<T> const& operator = ( Singleton<T> const& );

	// call this to create kernel

	static int Create() {
		if( SingletonPtr == 0 )
			SingletonPtr = ::new T;
		return 0;
	}
	// call this to kill it

	static int Destroy() {
		::delete SingletonPtr;
		SingletonPtr = 0;
		return 0;
	}

	static T* GetSingletonPtr() {
		return SingletonPtr;
	}
	static T& GetSingleton() {
		return *SingletonPtr;
	}
private:
	static T* SingletonPtr;
};

// static singleton pointer

template< typename T >
T* Singleton<T>::SingletonPtr = 0;

On the whole, I''m not a fan of templated canned singleton implementations. That being said, there are a couple of areas where your particular implementation can improve.

If a function is declared, but you don''t intend to define it, you might as well make it private. So move the copy constructor and assignment operator into a private portion of the class.

You''re double assigning SingletonPtr. Once in the constructor, once in the Create() function.

Why are you forcing global resolution on operator new/delete?

The GetSingleton() method should at least assert() on a null pointer. Dereferencing null pointer bad.

And you do realize this isn''t multi-threaded safe, right?

Might as well add another

(The old one I use to use from Game Programming Gems)

template <typename T> class GSINGLETON{private:    static T* GSINSTANCE;public:    GSINGLETON(void)	{		GSINSTANCE = (T*)((int)this + (int)(T*)1 - (int)(GSINGLETON <T>*)(T*)1);	}//    ~GSINGLETON(void)	{		GSINSTANCE = NULL;	}//    static T& GetGSINSTANCE(void)	{		return(*GSINSTANCE);	}//    static T* GetGSINSTANCE_PTR(void)	{		return(GSINSTANCE);	}};template <typename T> T* GSINGLETON <T>::GSINSTANCE = NULL;

-UltimaX-
Ariel Productions
|Designing A Screen Shot System|

"You wished for a white christmas... Now go shovel your wishes!"

[edited by - UltimaX on March 26, 2004 11:39:22 PM]