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

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
#include

class Race
{
private:
static long currentId;
long id;
std::string name;
std::vector> 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);
void AddHero(Hero &&

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:

• You cannot modify it because it is const

• 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".

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.

StackExchange Code Review Q#20729, answer score: 7