C++ Overloading << Operator problem

Question

I'm fairly noobish at C++, but very comfortable with pointers, dereferencing, etc. I'm having a problem with my overload of the << operator for a class, in that it compiles fine but crashes when run. It feels like an infinite loop, but I'm not certain. Here's the code, and any help is appreciated.

#include <string>
#include <iostream>

using namespace std;

class Person
{

private:

 string _name;
 Person* _manager;

public:

 Person(string name, Person *manager);
 Person(string name);

 friend ostream &operator<<(ostream &stream, Person &p);

};


Person::Person(string name, Person *manager)
{
 _name = name;
 _manager = manager;
}

Person::Person(string name)
{
 _name = name;
}

ostream &operator<<(ostream &stream, Person &p)
{
 Person* mgr = p._manager;

 stream << p._name << std::endl;
 stream << mgr->_name << std::endl;
 return stream;
}


int main()
{
 Person *pEmployee = new Person("John Doe Employee");
 Person *pManager = new Person("John Doe Manager", pEmployee);

 cout << *pEmployee;
 cout << *pManager;

 return 0;
}

Answer 1

In your constructor with just a single argument, you need to set _manager to NULL/0. Test for this in your operator<< and don't output mgr->name if it is NULL/0. As it stands, you are dereferencing an uninitialised pointer.

Person::Person(string name)
{
    _name = name;
    _manager = 0;
}

ostream &operator<<(ostream &stream, Person &p)
{
    Person* mgr = p._manager;

    stream << p._name << std::endl;
    if (mgr)
        stream << mgr->_name << std::endl;
    return stream;
}

There are a number of other things you could do better, such as using const references on arguments and using the constructor initialiser list, but they wont be the cause of your problem. You should also address ownership issues with the manager object you pass in to the constructor to ensure it does not get double-deleted.

Answer 2

your _manager pointer is not initialized upon constructing the first Person instance, hence referencing p._manager in your operator << crashes. Besides that, you have a memory leak since you call new but not delete.

Answer 3

Your Person instance *pEmployee does not have the _manager set. This may be a NULL pointer.

Answer 4

Person::Person(string name)
{
    _name = name;
}

Who is your manager?

Person::Person(string name) : _manager(NULL)
{
    _name = name;
}

ostream &operator<<(ostream &stream, Person &p)
{
 Person* mgr = p._manager;

 stream << p._name << std::endl;
 if (mgr != NULL) { stream << mgr->_name << std::endl; }
 return stream;
}

Answer 5

Your implementation of operator<< does not check if the _manager member is valid. If there is no manager, you should make this explicit by setting the pointer to 0. Otherwise the pointer value is undefined and accessing it will crash your program.

(1) Your Person class should set _manager to 0 (null pointer) if none is supplied in the constructor:

Person::Person(string name) : _name(name), _manager(0)
{
}

(2) In your operator<<, check the pointer before dereferencing it:

if (mgr) {
    stream << mgr->_name << std::endl;
}

Some hints for better code:

(1) Change your function arguments to accept const string& instead of string. This way, the string is not copied when calling the function/constructor, but passed as a constant reference.

(2) It is cleaner to let your operator<< accept a const reference for the Person as well, since it does not/should not modify the Person. This way, you can use the operator also in places where you have a constant Person.