0
\$\begingroup\$
  1. I have a base and derived class since I need to have different kinds of concrete classes.

  2. I cannot use the getInstance() function in the base class since I cannot make it both static and virtual.

class ITrafficEventsReturner {

    public:    virtual ~ITrafficEventsReturner() {} ;

    protected:

       ITrafficEventsReturner() { };

       // no definition for copy constructor and assignment operator  
       ITrafficEventsReturner(const ITrafficEventsReturner&);      
       ITrafficEventsReturner& operator=(const ITrafficEventsReturner&); };


    class ConcreteTrafficReturner : public ITrafficEventsReturner {

    public:
        static ConcreteTrafficReturner* getInstance() {
          if (myself == 0)
          {
             myself = new ConcreteTrafficReturner();
          }

          return myself;    }

       ~ConcreteTrafficReturner()
        {
          if (myself)
          {
             delete myself;
             myself = 0;
          }
        }

    private :    ConcreteTrafficReturner() : ITrafficEventsReturner()    {

       };

       static ConcreteTrafficReturner* myself; };

    ConcreteTrafficReturner* ConcreteTrafficReturner::myself = 0;

    int main() 
    {
       ITrafficEventsReturner* ConcreteTrafficReturner =     
            ConcreteTrafficReturner::getInstance();

       delete ConcreteTrafficReturner;
    };

This seems to be the possible solution :

class SingletonInterface
{
public:
   template <class T>
   static T& getInstance() 
   {
      return T::getInstance();
   }

private:
   SingletonInterface() { };
};

class TrafficGenerator 
{
public:

   static TrafficGenerator& getInstance()
   {
      static TrafficGenerator obj;
      return obj;
   }

   void whatAmI()
   {
      std::cout << " I am Traffic Generator" << std::endl;
   }

private:

   TrafficGenerator () {};
};

int main() 
{

   TrafficGenerator& hTG = SingletonInterface::getInstance<TrafficGenerator>() ;
   hTG.whatAmI();
};
\$\endgroup\$
  • 1
    \$\begingroup\$ I get compile errors when I try to run this. You should fix those before asking for review. \$\endgroup\$ – Brythan Mar 6 '15 at 18:21
  • \$\begingroup\$ What compiler are you using? I am using visual studio 2005 and it compiles fine for me. \$\endgroup\$ – Eternal Learner Mar 6 '15 at 20:47
  • \$\begingroup\$ Wait, I think the error was because it was incorrectly copy/pasted. I've corrected it now. \$\endgroup\$ – Eternal Learner Mar 6 '15 at 20:48
2
\$\begingroup\$

Please never do this again:

    static ConcreteTrafficReturner* getInstance() {
      if (myself == 0)
      {
         myself = new ConcreteTrafficReturner();
      }

Problem 1:

You are not providing a concept of ownership. The user of your class has to actually read the code to understand that you are not only returning the pointer but ownership of the pointer.

Problem 2:

You are returning a pointer. So who decides when it should be deleted. What happens if the user of your class forgets to call delete!

A better way to implement this is:

    static ConcreteTrafficReturner& getInstance()
    {
       static ConcreteTrafficReturner    instance;    // created the first time
                                                      // get instance is called.
                                                      //
                                                      // automatically destroyed
                                                      // with other static storage
                                                      // duration objects.

        return instance;
    }

But really the above is a text book example (not a real world example) of a singelton. The singelton pattern is actually dangerous in that it binds your implementation to an object with mutable state. As such it should not be used in isolation it should be used in conjunction with a creator pattern (A factory is usually a good choice).

Note: Singelton is a design pattern. But in a lot of places it is also considered and anti-pattern so be very careful with its usage.

Template problem

Though technically correct. You may find problems with this design when using shared libraries. As each shared library may have its own instantiation of the templated object that is not shared. Thus you actually have multiple instances of your singelton.

Summary:

Personally I would not use a singelton (I think you can use other techniques). But if you have to use a singelton then it would look like this:

class MySingInterface
{
    public:
       virtual ~MySingInterface() {}  // Never foget the virtual destructor of
                                      // a class with virtual functions.
       // Other interfaces
};
class MySingFactory
{   
    public:
        virtual ~MySingInterfaceFactory() {}
        virtual MySingInterface& createMySingInterface() = 0;
};

class MySingBuilder
{
    static std::vector<MySingFactory*>   factories;             // This could be a map
                                                                // of factories or 
                                                                // anything else that
                                                                // makes sense to your
                                                                // project.


       static MySingInterface& buildInstance()
       {
             // decide what factory to use.
             // then call build (EG if you are in debug mode or unit test mode
             // you may want to build a different type of object from normal).
             //
             // I have shown an example where you can register multiple
             // factories but that may be overkill for you. Maybe you only need
             // to register one factory (which you decide in main and depends
             // on input arguments or config options or environment variables etc...)

             return factories[someIndex]->createMySingInterface();
       }
    public:
       MySingInterface&  getInstance()
       {
           static MySingInterface& instance = buildInstance();
           return instance;
       }

       static void addFactory(MySingFactory& factory)
       {
           factories.push_back(&factory);
       }
};

MySingFactoryForTest   mySingFactoryForTest;
MySingFactoryForNormal mySingFactoryForNormal;

int main()
{
    if (getenv("UNIT_TEST") != nullptr)
    {
        MySingBuilder::addFactory(mySingFactoryForTest);
    }
    else
    {
        MySingBuilder::addFactory(mySingFactoryForNormal);
    }

    MySingInterface& mSI = MySingInterfaceBuilder::getInstance();
}
\$\endgroup\$
  • \$\begingroup\$ using static object is not always possible. the ownership transfer method is legit a due to original singleton pattern (because it assumed that ownership is passed to process). Creating object in static duration storage is illegal if framework that requires certain order of initialization, one like Qt. In Qt any class in QtGui would be created after QApplication object was created. Ownership might be restricted by restricting access to destructor and creating mechanism of its deletion which is controlled through the object (also was described in original source of singleton pattern). \$\endgroup\$ – Swift Feb 18 '17 at 12:01
  • \$\begingroup\$ @swift. The use of static here is perfect for exactly that reason. static function members are not created (constructed) until the first time the function is called. Thus it enforce the order of construction (just like the original code) but also guarantees destruction (unlike the original code). Also note that order of initialization problem is really a "red herring". Its a problem for people that don't know the issue but once you understand the issue enforcing an order is trivial and well discussed: stackoverflow.com/a/335746/14065 \$\endgroup\$ – Martin York Feb 19 '17 at 15:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.