Singleton class - Really confused

[phorce]

Hello,

So basically, I am trying to create a system based on the Singleton Pattern and I'm confused.

If I have an Animal class, as well as a Cat class which is derived from Animal and then using a Singleton class will create a single object of which-ever class has been selected (dog, horse, cat etc..):

Animal.h

class Animal {

    public:
        virtual int age() = 0;
        virtual void breed() = 0;   
};       

Cat.h

#include "Animal.h"

class Cat : public Animal {
  public:
    Cat();
    int age();
    void breed();  
};

Cat.cpp

Cat::Cat(){};

int Cat::age()
{
    return 10; 
}
void Cat::breed()
{


}      

Singleton.h

#include "Animal.h"    
#include <iostream>

using namespace std; 

class Singleton 
{
    private: 
        static bool instanceFlag;
        static Singleton *single;
        Singleton()
        {

        }
    public:
        static Singleton* getInstance();
        Animal *method(string something);
        ~Singleton()
        {
            instanceFlag = false;
        }   


};

Singleton.cpp

#include "Singleton.h"
#include <iostream>

using namespace std;

bool Singleton::instanceFlag = false;
Singleton* Singleton::single = NULL;

Singleton* Singleton::getInstance()
{
    if(!instanceFlag)
    {
       single = new Singleton();
       instanceFlag = true;
       return single;
    }else{
      return single;
    }  
}    

Animal *Singleton::method(string something)
{
    if(something == "cat")
    {
       // new Cat instance
    } 
    // etc
}

main.cpp

#include <iostream>
#include "Singleton.h"
using namespace std;    


int main()
{        

    Singleton *sc1;
    sc1 = Singleton::getInstance();
    sc1->method("Cat");
    return 0;
}

Anyone know where I am going wrong? Why I can't create a new Cat and call it's methods...?

Any help would be amazing :)!

[VernonDozier]

Why I can't create a new Cat

I'm searching for the phrase "new Cat" and not seeing it, except in a comment. Also, capitalization counts. "Cat" and "cat" are not the same...

sc1->method("Cat");
if(something == "cat")

Not directly relating to your problem, but still needlessly complicating your code is that you have TWO flags to test whether a singleton object exists. You have a pointer to a Singleton called single and you have a boolean flag called instanceFlag. Think about it. If instanceFlag is true, single had better not be NULL, and if instanceFlag is false, single needs to be NULL or you have a memory leak. You're keeping track of TWO variables and thus having a problem since you set instanceFlag to false, but you never set single back to NULL. Just keep track of single and get rid of the boolean flag.

You also have a problem here.

Animal *Singleton::method(string something)
{
    if(something == "cat")
    {
       // new Cat instance
    } 
    // etc
}

There are a few problems with this code. One, this is not a constructor, yet it CALLS a constructor every time I call this function (once you uncomment the "new Cat" part). Presumably I need to keep track of the pointer that is returned, then eventually delete the Cat that it points to. Thus if I call method three times, I have to keep track of three Cat pointers and then delete them. Presumably the whole point of the Singleton was to make memory management easy and make sure that there was only one Animal that was ever created. Hence the Singleton constructor should create the Cat object and the Singleton destructor should delete it, not some non-static function called "method".

Animal is polymorphic. Right now there is only one type of animal: cat. But suppose there is a second type(again, one would assume there IS a second type. Otherwise, why bother with polymorphism?). The Singleton is to ensure that only one animal exists. So perhaps you need your getInstance() function to take the type of Animal as a parameter. You should probably also have a static deleteInstance() function containing any destructor calls. And you'd better be careful that you don't create your one Singleton object as a Cat, then need it to be a Dog later, or at least have a plan to handle that scenario if it is allowed.

Anyway, the short answer is that your main problem is the capitalization problem. I think you have an overall design problem though. Singletons can be very useful, but you need to understand precisely what you are aiming for and how they should be used.

[mike_2000_17]

Just to add a remark, the more typical (hassle-free and safe) implementation of a singleton class is as follows:

// in header:

class Singleton 
{
    private: 
        Singleton() {
          // ...
        }
        Singleton(const Singleton&); // no implementation. non-copyable
        Singleton& operator=(const Singleton&); // no implementation. non-copyable
    public:
        static Singleton& getInstance();  // get instance by reference
        Animal *method(string something);
};

// in cpp file:

Singleton& Singleton::getInstance() {
    static Singleton inst; // just a single static instance of the Singleton object.
    return inst;
}
//...

[phorce]

Thank you for both of your replies :)!

Basically, I managed (after a lot of shouting and banging things!) to get this working, and it does the expected output BUT I am just wondering if this is the best implementation for such a singleton problem?

#include <iostream>
using namespace std;

class Animal {

    public:
        virtual int age() = 0;
        virtual void speak() = 0;
};

class Cat : public Animal
{
    public:
            int age() { return 4; }
            void speak() { cout << "MEOW"; }

};

class Dog : public Animal {

    public:
       age() { return 4; }
       void speak() { cout << "WOOF!!"; }


};

class Horse : public Animal {

    public:
        int age() { return 4; }
        void speak() {cout << "MERRR"; }

};

class Singleton
{
    public:
        Animal *newAnimal(string theTypeOfAnimal);

        static Singleton *instance()
        {
            static Singleton instance;
            return &instance;
        }
      private:      
      static Animal* pinstance;
};

Animal* Singleton::pinstance = 0;


Animal *Singleton::newAnimal(string theTypeOfAnimal)
{
    if(theTypeOfAnimal == "Cat")
    {
        this->pinstance = new Cat;
    }else if(theTypeOfAnimal == "Dog")
    {
        this->pinstance = new Dog;

    }else if(theTypeOfAnimal == "Horse")
    {
        this->pinstance = new Horse;
    }
    return pinstance;
}  

int main(int argc, char *argv[]) {

    //Animal *pAnimal = NULL;

    Animal *pAnimal = Singleton::instance()->newAnimal("Horse");

    cout << pAnimal->age() << endl;
    pAnimal->speak();


}

Thank you again for the help :)!

[vijayan121]

The singleton design pattern is unique in that it's a strange combination: its description is simple, yet its implementation issues are complicated. - Andrei Alexandrescu in 'Modern C++ Design'

If Animal is a singleton - that is a second object of type Animal cannot exist - its singletonness can be enforced by the class Animal and by the class Animal alone.

This is one typical way of ensuring that one - and only one - instance of an object of dynamic type 'some unknown derived class of Animal' can exist at any point of time.

    //////////////  animal.h ///////////////////

    struct Animal
    {
        static Animal& get_it() ;

        virtual ~Animal() ;
        virtual int age() const = 0 ;
        virtual void speak() = 0 ;

        Animal( const Animal& ) = delete ; // non-copyable
        Animal( Animal&& ) = delete ; // and non-movable

        protected: Animal() ;
    };

--------------------------------

//////////////  animal.cc ///////////////////

#include "amnimal.h"
#include <atomic>
#include <mutex>
#include <stdexcept>

namespace
{
    std::atomic<Animal*> singleton(nullptr) ;
    std::mutex guard ;
}

Animal::Animal() // a constructor of a derived class will invoke this constructor
{
    if(singleton) throw std::logic_error( "Animal is a singleton" ) ;
    else singleton = this ;
}

Animal::~Animal() { singleton = nullptr ; }

Animal& Animal::get_it()
{
    std::lock_guard<std::mutex> lock(guard) ;

    if( !singleton ) throw std::logic_error( "singleton instance not created" ) ;
    else return *singleton ;
}

------------------------------

///////////////// main.cc ///////////

#include "amnimal.h"
#include <iostream>

int main()
{
    struct Zebra : Animal
    {
        virtual int age() const override { return 3 ; }
        virtual void speak() override { std::cout << "zebra: whinny\n" ; }
    };

    Zebra the_only_animal ;

    Animal& a = Animal::get_it() ;
    a.speak() ;

    struct Dog : Animal
    {
        virtual int age() const override { return 2 ; }
        virtual void speak() override { std::cout << "dog: bark\n" ; }
    };

    try
    {
        Dog another_animal ;
        Animal& b = Animal::get_it() ;
        b.speak() ;
    }
    catch( const std::exception& e ) 
    { std::cerr << "*** error: " << e.what() << '\n' ; }
}

Note: guarding against race conditions is required only if concurrency is an issue.

[VernonDozier]

BUT I am just wondering if this is the best implementation for such a singleton problem?

To fully test your Singleton class, you need to have an Animal constructor and destructor with some debugging output in it that shows when they are called. You also need to write a more thorough testing program in main. You have only one call to

Singleton::instance()->newAnimal("Horse");

so that can't test a Singleton class. You need at least two calls and you need to test to make sure that there is exactly ONE call total to the Animal constructor.

In my view, the line above (from main) means that the Singleton is incorrect. A Singleton class should not have a public method called "newAnimal". That's far too close to "new Horse()". The whole point of a Singleton class is that you don't have the responsibility (from main) of keeping track of whether you need to create a new Horse or simply use the one you already have. That needs to be hidden and hence private. You have the public "instance" function. That function needs to keep track of the single object and call the Cat constructor if needed. Thus any calls to the constructor needs to test the pinstance variable before calling a constructor. If pinstance is NULL, you call a constructor. If not, you don't. You set the pinstance variable in your code, but you never test it. Hence it serves no purpose as far as I can tell.

[L7Sqr]

phorce are you familiar with templates at all? They are a much nicer way to have the type-based behavior you are looking for. The question is independent of the singleton class (which could still be used).

You could transition to something similar to

// Two ways to create a Dog
Animal * a = AnimalFactory::Create< Dog >(); 
Dog * d = AnimalFactory::Create< Dog >();

// or a Cat
Cat * c = AnimalFactory::Create< Cat >();

Where the body of the Create method would be as simple as

struct AnimalFactory {
    template< class T >
    static T * Create () { return new T; }
};

[phorce]

L7Sqr I like this implementation actually :)!

Would this allow me to create a Singleton though? For example, it looks like I can create multiple objects so I need a way that only ONE object can be created?

[L7Sqr]

Well, my comment was more on the implementation of the factory and less on the singleton. Nothin prevents you from making the AnimalFactory a singleton but the Create method is static so it really wouldn't serve much of a purpose. I'm unsure that is what you want, however. Are you looking to have the AnimalFactory as a singleton or the classes it creates be singletons? For instance, when you call

Dog * d1 = AnimalFactory::Create< Dog >();
Dog * d2 = AnimalFactory::Create< Dog >();

You want that d1 nad d2 point to the same object?