CODEWITHSUNDEEP
c++copy-constructorstdstring
conciliator
conciliator

Reputation: 6138

Why doesn't my copy constructor work?

This is probably a no-brainer, but I'm a little stumped. I've had some trouble with my vectors not behaving nicely, and now it looks like I've found the culprit. Here's a watered down version of my Player class. 

class Player {
private:
    std::string _firstName;
    std::string _lastName;
public:
    Player(std::string firstName, std::string lastName) {
        _firstName = firstName;
        _lastName = lastName;
    };
    Player(const Player& otherPlayer) {
        _firstName = otherPlayer._firstName.c_str();
        _lastName = otherPlayer._lastName.c_str();
        std::cout << "Created " << _firstName << " " << _lastName << std::endl; // Why doesn't _firstName and _lastName contain anything?
    };
    std::string GetName() { return _firstName + " " + _lastName; };
};

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

    Player player1 = Player("Bill", "Clinton");
    Player player2 = Player(player1);

    std::cout << "Player: " << player2.GetName() << std::endl;

    return 0;
}

The output is a meager Player:. I'm not sure why my copy constructor doesn't do what I want it to do, in particular in light of advice such as this (Zac Howland's comment accounts for the c_str();-part). Am I violating the rule of three (which, btw, I still haven't totally gotten my head around)? I'd be really grateful if anyone could point me in the right direction!

Upvotes: 0

Views: 296

Answers (1)

Pierre Fourgeaud
Pierre Fourgeaud

Reputation: 14510

It works for me : http://ideone.com/aenViu

I just addded :

#include <iostream>
#include <string>

But there is something I don't understand :

_firstName = otherPlayer._firstName.c_str();
_lastName = otherPlayer._lastName.c_str();

Why the .c_str() ? You convert the string to char* to assign it to a new string?

EDIT : From the comment, Zac Howland pointed : "Prior to C++11, if you wanted to ensure your string was copied (instead of reference counted), you had to use the c_str() method to force it to copy the string. The new standard eliminates that, but if he's using an older compiler, or one that hasn't fully implemented C++11 yet, it will ensure a deep copy."

Just do :

_firstName = otherPlayer._firstName;
_lastName = otherPlayer._lastName;

And, do you really need this copy constructor ? The default would do what you want I think...

Also, instead of assigning the members :

Player(std::string firstName, std::string lastName) {
    _firstName = firstName;
    _lastName = lastName;
}

use the member-initialization-list instead :

Player(std::string firstName, std::string lastName) :
    _firstName( std::move(firstName) ),
    _lastName( std::move(lastName) )
{}

In the first case the string's default constructor is called and then string's copy-assignment operator, there could definitely be (minor) efficiency losses compared to the second case, which directly calls the copy-constructor.

Last thing, when it is possible, does not pass values as method arguments, pass references and even const references when they don't need to be modified :

Player( const std::string& firstName, const std::string& lastName )
//      ^^^^^            ^            ^^^^^            ^
    : _firstName( firstName )  // no move here, since args are constant references
    , _lastName( lastName )
{}

Working live example of all the modifications.

Upvotes: 4

Related Questions