Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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);
    void AddHero(Hero &&hero);
    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;
}

void Race::AddHero(Hero &&hero)
{
    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");
    nightElf.AddHero(std::move(elvesHero1));
    nightElf.AddHero(std::move(elvesHero2));
    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.

share|improve this question
add comment

2 Answers

up vote 7 down vote accepted

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.

share|improve this answer
    
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
add comment

In addition to @Yuushi

I just want to emphasis this bit:

Hero elvesHero1("Demon hunter", 8);

nightElf.AddHero(std::move(elvesHero1));

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
 {
      void AddHero(std::shared_ptr<Hero>& hero);
 };
 int main()
 {
     // STUF
     std::shared_ptr<Hero> = new elvesHero1("Demon hunter", 8);

     Race nightElf("Night elf");
     nightElf.AddHero(std::move(elvesHero1));
 }

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?

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.