Using singleton in c++

[janith.amarawickrama]

I created following classes in my program.

Way.h

class Way {
private:
    std::string id;
    std::string name;
public:
    Way();
    Way(const Way& orig);

    void SetName(std::string name) { this->name = name; }
    std::string const & GetName() const { return name; }

    void SetId(std::string id) { this->id = id; }
    std::string const & GetId() const { return id; }
};

way.cpp

#include "Way.h"

Way::Way() {
}

Way::Way(const Way& orig) {
}

AreaDataRepository.h

class AreaDataRepository {
private: 
    Way onGoingWay;
public:
    AreaDataRepository();
    AreaDataRepository(const AreaDataRepository& orig);
    ~AreaDataRepository();

    static AreaDataRepository & Instance()
    {
        static AreaDataRepository singleton;
        return singleton;
    }

    void SetOnGoingWay(Way onGoingWay)  { this->onGoingWay = onGoingWay;  }
    Way const & GetOnGoingWay() const { return onGoingWay; }
};

AreaDataRepository.cpp

#include "AreaDataRepository.h"
#include "../common/Common.h"

AreaDataRepository::AreaDataRepository() {
}

AreaDataRepository::AreaDataRepository(const AreaDataRepository& orig) {
}

AreaDataRepository::~AreaDataRepository() {
}

in program main method

Way wayNode
wayNode.SetId("1234");
wayNode.SetName("jan");

AreaDataRepository::Instance().SetOnGoingWay(wayNode);

std::cout<<"Id ->>>>>>>>>>>>>" <<AreaDataRepository::Instance().GetOnGoingWay().GetId() << std::endl;
std::cout<<"Name ->>>>>>>>>>>>>" <<AreaDataRepository::Instance().GetOnGoingWay().GetName() << std::endl;

but this print nothing and i try to debug and get the values, vales are empty for OnGoingWay.
I need some help to sort out this.
thanks in advance!

[StuXYZ]

You error seems to be in the copy constructor/assignment operator of way.

If you add :

Way::Way(const Way& orig) : 
  id(orig.id),name(orig.name)
{}

Way& Way::operator=(const Way& A)
  {
     if (this!=&A)
       {
          id=A.id;
          name=A.id;
        }
     return A;
   }

All works as I would expect. [Note you also have to add a declaration to the class definition of Way for the assignment operator.

So let us look at WHY that is:

The problem comes from the methog you use to write a wayNode to the AreaDataRepository. First off you write

void SetOnGoingWay(Way onGoingWay) { this->onGoingWay = onGoingWay; }

This isn't good! Way is a large class why are you not using references. This is what I would have expected to see:

 void SetOnGoingWay(const Way& onGoingWay)  
   {
     this->onGoingWay = onGoingWay;  
   }

So in the first, we have a local constructor (copy constructor) called to make onGoingWay and then an assignment operator called in the actual function to do this->onGoingWay=onGoingWay. The second is an assignment operator call. [Note: Most optimizers remove one of those calls]. But that means that your version of the copy constructor does not set id or name, but instead relies on the default std::string consturctor which gives an empty string.

Other problems:

While we are looking -- if AreaDataRepository is to be used as a singleton - then make the constructor, copy constructor and assignment operator private because as it stands you can circumvent your singleton requirement.

[rubberman]

A couple of minor issues with the assignment operator in StuXYZ's example:

Way& Way::operator=(const Way& A)
{
    if (this!=&A)
    {
        id=A.id;
        name=A.id;
    }
    return A;
}

should be this:

Way& Way::operator=(const Way& A)
{
    if (this!=&A)
    {
        id=A.id;
        name=A.name;
    }
    return *this;
}