# Am I using copy ctors/move ctors/shared_ptr correctly?

Here is my simple example: there is a class A with few primitive members and a class B with few primitive members but also a collection of objects of type A.

This is my A class:

#pragma once

#include<string>

class Hero
{
private:
long id;
std::string name;
int level;
static long currentId;
public:
Hero();
Hero(std::string name, int level);
Hero(const Hero &hero);
Hero(Hero &&hero);
Hero& operator=(const Hero &hero);
Hero& Hero::operator=(const Hero &&hero);
long GetId() const { return this->id; }
std::string GetName() const { return this->name; }
int GetLevel() const { return this->level; }
void SetName(const std::string &name);
void SetLevel(const int &level);
};

#include"Hero.h"

long Hero::currentId = 0;

Hero::Hero(std::string name, int level):name(name), level(level), id(++currentId)
{

}

Hero::Hero():name(""), level(0), id(++currentId)
{

}

Hero::Hero(const Hero &hero):id(hero.id), name(hero.name), level(hero.level)
{

}

Hero::Hero(Hero &&hero):id(hero.id), name(hero.name), level(hero.level)
{

}

Hero& Hero::operator=(const Hero &hero)
{
this->id = hero.id;
this->name = hero.name;
this->level = hero.level;
return *this;
}

Hero& Hero::operator=(const Hero &&hero)
{
this->id = hero.id;
this->name = hero.name;
this->level = hero.level;
return *this;
}

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

void Hero::SetLevel(const int &level)
{
this->level = level;
}


This is my B class:

#pragma once

#include"Hero.h"
#include<vector>
#include<memory>

class Race
{
private:
static long currentId;
long id;
std::string name;
std::vector<std::shared_ptr<Hero>> heroes;
Race(const Race &race); //disable copy constructor
Race& operator =(const Race &race); //disable assign operator
public:
Race();
explicit Race(std::string name);
std::string GetName() const { return this->name; }
void SetName(std::string name);
std::vector<std::shared_ptr<Hero>> GetHeroes() const { return heroes; }
};

#include "Race.h"

long Race::currentId = 0;

Race::Race(std::string name):name(name), heroes(std::vector<std::shared_ptr<Hero>>()), id(++currentId)
{

}

Race::Race():name(""), heroes(std::vector<std::shared_ptr<Hero>>()), id(++currentId)
{

}

void Race::SetName(std::string name)
{
this->name=name;
}

{
heroes.push_back(std::make_shared<Hero>(hero));
}


And this is my main:

#include "stdafx.h"

#include"Race.h"

int _tmain(int argc, _TCHAR* argv[])
{
Hero elvesHero1("Demon hunter", 8);
Hero elvesHero2("Warden", 4);
Race nightElf("Night elf");
std::vector<std::shared_ptr<Hero>> heroes=nightElf.GetHeroes();
return 0;
}


One remark: When the debugger hits nightElf.AddHero(std::move(elvesHero1)); it enters Hero's copy constructor. Shouldn't it go through move constructor?

I just want start on the right foot, so I am waiting for your tips.

LE: If I modify the AddHero method like this:

void Race::AddHero(Hero &&hero)
{
heroes.push_back(std::make_shared<Hero>(std::move(hero)));
}


it gives the parameter std:move(hero) instead of hero. It enters move constructor instead of copy constructor.

-

Firstly, in your header file, you don't need the Hero:: qualification on your move assignment operator. Secondly, you are defining it as Hero& Hero::operator=(const Hero &&hero);. Think about what this is saying - it's saying if you have an rvalue reference to a Hero object, then:

1. You cannot modify it because it is const
2. You want to utilize move semantics (specifically, std::move) which will modify the original object and leave it with an empty value.

Hence, your move constructor should always be of the form Hero& Hero::operator=(Hero&& hero). The implementation should also be using std::move:

Hero& operator=(Hero&& hero)
: id(std::move(hero.id),
name(std::move(hero.name),
level(std::move(hero.level)
{ }


For your AddHero methods, a Hero&& hero might be an rvalue reference, but without the std::move for its elements, you're basically saying "take an rvalue reference to Hero, and call copy constructors on each of its elements when you utilize make_shared<Hero>".

Note that moving vs copying for basic types like int or long won't have any performance difference.

That being said, you're doing a lot of work here for no benefit. The rule is, unless you have written a custom destructor, you don't need to write custom copy constructors, copy assignment operators or move assignment operators - the compiler will generate defaults which are correct. It is only when you are managing memory (hence have a delete or delete[] in a destructor somewhere) that you need to implement these.

Finally, a few other small points. Your constructors should be taking strings by const &, ie, explicit Race(const std::string& name). Usage of this-> to return member variables is a bit unidiomatic in C++, likewise in copy constructors and the like, instead of this->id = hero.id; it's more idiomatic to simply write id = hero.id;.

I'd suggest reading the Rule of Three post on stackoverflow to get a better understanding of copy construction and copy assignment.

-
As we are in C++11 land, I would strongly recommend moving from Rule of Three to Rule of Zero. Also, it would be nice to mention the Quasi Classes paper. But +1 anyway. –  Griwes Jan 24 '13 at 17:08

I just want to emphasis this bit:

Hero elvesHero1("Demon hunter", 8);



Here elvesHero1 is an object. When you call AddHero() you create a new object that is shared. So you now have two independent "Demon hunter" objects (the first is automaticelvesHero1 the other is dynamic and controlled by a shared pointer inside nightElf.

Is this really what you wanted?

I assume that you want to create Hero(s) dynamically. Thus usually they are going to be created with new and controlled by a smart pointer. Thus the interface to Race should reflect this and accept the appropriate smart pointer in its interface AddHero().

Thus I would expect to see:

 class Race
{
};
int main()
{
// STUF
std::shared_ptr<Hero> = new elvesHero1("Demon hunter", 8);

Race nightElf("Night elf");

The next question you want to ask is std::shared_ptr the correct smart pointer. I can't answer this as I don't know enough about the app. Do you expect multiple ownership of he object? Or is Race going to have exclusive ownership and share references externally?