Looks fine (its better than the static singleton approach). I do prefer to have somehtign actually own it and not have it created on first use and destroyed in some random order at exit. I would just use the constructor/destructor instead of create instance/destroy instance. Though perhaps your idea is more explicit in describing whats going on. I would definatly make it fail outright if you try to create an instance when one already exists (rather than just returning the current instance), otherwise, whoever tried creating it will assume they now own it and are responsible for destroying it too.

static App *instance = NULL;

class App
{
public:
App()
{
assert(instance == NULL); // Throw some error, force it to crash so the programmer only ever calls it once by design.
instance = this;
}
~App()
{
assert(instance == this);
instance = NULL;
}
static App* GetInstance()
{
// Throw an error if no instance exists
return instance;
}
};

I would first think about why you need to make this App class globally accessable though.

Note that your code becomes a lot simpler when not using singletons here:class App {
public:
App() : m_Logger("MyLogger"), m_Memory() { m_Logger.Log("Starting up"); }
~App() : { m_Logger.Log("Shutting down"); }
private:
Logger m_Logger;
MemoryManager m_Memory;
};

//Usage:
int main() {
App a;
// Now App and all app's members are usable
a.foo();
...
}No concerns about order of creation/destruction either...

What does all the added complexity of using the singleton pattern give you here (besides the disadvantage of turning things into global variables)?

Thanks for your responses.

@Nanoha: Just out of curiosity: Why do you use a module static global variable: static App *instance = NULL; ?
Is that intentional or did you mean a class variable: App* App::instance = NULL;

I did mean static, that bit would usually go in the implementation(cpp) and not the header. It could go in the class definition.

If you want the logger to be globally accessable, then make just the logger globally accessable (and not the App). If you want to enforce the App as a singleton, then just have a flag saying if an instance has been created and force it to crash/throw an exception.

class App
{
private:
static bool instanceExists = false;
public:
App()
{
// Check instance doesn't exist
if(instanceExists) { throw somehting; }
instanceExists = true;
}
~App()
{
instanceExists = false;
}
};

C++ singletons seem really silly to me. Why not just use a namespace?:
App::Log(const std::string& s)
Or create a single instance of the class, using what you would normally think of the class name:
class App_
{
void Log(const std::string& s);
};
App_* App = new App_;
Why all these drastic measures to try to conform to a paradigm C++ doesn't support?

Why all these drastic measures to try to conform to a paradigm C++ doesn't support?

The most important reason whenever I use it: not having it created at some random point before main.

A somewhat constructed example.


class AutoRegister
{
AutoRegister() { globalMap[id] = this; }
};


AutoRegister globalObject;


This can crash horribly, if globalMap is in some other .cpp, since you have no control if globalObject or globalMap is created first. In one case you're lucky, in the other you will waste a lot of time wondering why accessing the map will fail badly.


map<x,x>& registryMap()
{
static map<x,x> instance;
return instance;
}


Now it can't crash, because calling "registryMap()[id] = this" will make sure that the map is instanced before you actually access it for the first time.

Personally I think it partially defeats the purpose of a singleton, if it has to be explicitely created and destroyed. Sure, you still make sure there can be one and only one (kind of), but all the automatism and safety is ripped right out of it.

Obviously, as long as you never ever access a global object from another global objects constructor, this shouldn't become an issue. A common example however would be static registration with a factory.

Making the logger a global object could suffer the same fate, if object constructors try to use the logger which tends to happen in the kind of classes that are tempting to make global as well (class MyEngine, etc.)

In this simple case you might notice that I don't bother wrapping the map in a dummy class, just to make some static "getInstance()" function.

class AutoRegister
{
AutoRegister() { globalMap[id] = this; }
};
AutoRegister globalObject;

Going off topic, but when you least expect it, the linker is going to dead-strip that code and it's not going to make it into the final EXE, leaving you wondering why auto-registration has suddenly stopped working for certain classes...

Running code before main is evil.