Why must I return a pointer to an object and not an object?

[trillionaire]

Hi there, I have written a simple program which I know a solution to, I simply don't understand why the version below doesn't work. I have investigated the answer to roughly how it goes wrong, I just don't know why! Where the problems begin is commented in the last six lines of code, and it's frustrating as I believe it is pretty elegant this way... only it does work. More remarks follow the listing:

#include <iostream>

class Point
{
private:
    int *itsX;
    int *itsY;
public:
    Point();
    Point(int x, int y);
    ~Point();
    void SetX(int x);
    void SetY(int y);
    void SetXY(int x, int y);
    int GetX() const;
    int GetY() const;
};

Point::Point()
{
    itsX = new int(0);
    itsY = new int(0);
}

Point::Point(int x, int y)
{
    itsX = new int(x);
    itsY = new int(y);
}

Point::~Point()
{
    delete itsX;
    delete itsY;
}

void Point::SetX(int x)
{
    *itsX = x;
}

void Point::SetY(int y)
{
    *itsY = y;
}

void Point::SetXY(int x, int y)
{
    *itsX = x;
    *itsY = y;
}

int Point::GetX() const
{
    return *itsX;
}

int Point::GetY() const
{
    return *itsY;
}

class Rectangle
{
private:
    Point *itsLowerLeft;
    Point *itsUpperRight;
public:
    Rectangle();
    Rectangle(int llx, int lly, int urx, int ury);
    ~Rectangle();
    void SetLowerLeft(int x, int y);
    void SetUpperRight(int x, int y);
    Point GetLowerLeft() const;
    Point GetUpperRight() const;
    int GetArea() const;
};

Rectangle::Rectangle()
{
    itsLowerLeft = new Point(0, 0);
    itsUpperRight = new Point(1, 1);
}

Rectangle::Rectangle(int llx, int lly, int urx, int ury)
{
    itsLowerLeft = new Point(llx, lly);
    itsUpperRight = new Point(urx, ury);
}

Rectangle::~Rectangle()
{
    delete itsLowerLeft;
    delete itsUpperRight;
}

void Rectangle::SetLowerLeft(int x, int y)
{
    itsLowerLeft->SetXY(x, y);
}

void Rectangle::SetUpperRight(int x, int y)
{
    itsUpperRight->SetXY(x, y);
}

Point Rectangle::GetLowerLeft() const
{
    return *itsLowerLeft;
}

Point Rectangle::GetUpperRight() const
{
    return *itsUpperRight;
}

int Rectangle::GetArea() const
{
    return(itsUpperRight->GetX() - itsLowerLeft->GetX()) *
            (itsUpperRight->GetY() - itsLowerLeft->GetY());
}

int main(int argc, char *argv[])
{
    Point origin(0,0);
    std::cout << "origin.x = " << origin.GetX() << std::endl;
    std::cout << "origin.y = " << origin.GetY() << std::endl;
    Point *pOrigin = new Point(0, 0);
    std::cout << "pOrigin->x = " << pOrigin->GetX() << std::endl;
    std::cout << "pOrigin->y = " << pOrigin->GetY() << std::endl;
    Rectangle square(0, 0, 1, 1);
    //un-comment line below to cause crash:
    //std::cout << "square llx = " << square.GetLowerLeft().GetX() << std::endl;
    Rectangle *pSquare = new Rectangle(0, 0, 1, 1 );
    //un-comment both lines below to cause crash:
    //std::cout << "pSquare llx = " << pSquare->GetLowerLeft().GetX() << std::endl;
    //std::cout << "pSquare llx = " << pSquare->GetLowerLeft().GetX() << std::endl;
}

When I threw the problem up for grabs a programmer at Lionhead (TheBag) pointed out that if I alter my Get functions in the rectangle class so they return a pointer to each corner I can make it work. He's right. I only don't understand why, when I try to return the Point object, as I do above, the program tends to crash. Why is this illegal, particularly as it compiles ok?

All I can tell is that it seems to call the Point destructor After (for some reason) it has completed each cout statement. I know this as I have tried putting couts in every separate function in the program to see where it's at. If I remove the deletes in the Point destructor it carries on without crashing, but seems silly as I'm supposed to be deleting my pointers in my destructors, so the question is, please tell me why my program doesn't work when I remove the slashes from the comments above?

[Narue]

Your Point class doesn't have a copy constructor. Passing objects of that class around by value is an exceptionally bad idea since the data members are dynamically allocated. Default copy construction won't work in this case.

[trillionaire]

Ah, ok, so I'm a little out of my depth. I haven't got to the copy constructor bit in the manual. Thank you so much for the response. Although you've also thrown up something else I don't understand: I don't get why passing objects of the class around by value is a bad idea because I don't know what dynamically allocated means... can you tell me please?

[Narue]

Ah, ok, so I'm a little out of my depth. I haven't got to the copy constructor bit in the manual. Thank you so much for the response. Although you've also thrown up something else I don't understand: I don't get why passing objects of the class around by value is a bad idea because I don't know what dynamically allocated means... can you tell me please?

itsX and itsY are pointers, and you allocate memory to those pointers with new . That's what dynamically allocated means. The default copy constructor just does a shallow copy, much like this:

Point(const Point& rhs)
{
    itsX = rhs.itsX;
    itsY = rhs.itsY;
}

But what this does is copy the pointers, not the memory pointed to by the pointers. The end result is you have two objects that refer to the same memory. If one of those objects goes out of scope, the destructor deletes the memory and the other object now references deleted memory.

Typically when you accidentally cause objects to share memory, that's really a bad thing. ;) So you need to use a custom copy constructor which allocates memory and copies data:

Point(const Point& rhs)
{
    itsX = new int(*rhs.itsX);
    itsY = new int(*rhs.itsY);
}

This way the memory is completely owned and managed by each object.