Weapon and Hero classes

Question

For me as a complete newbie to the world of C++, getters and setters are a rather confusing topic. There seems to be a wide range of opinions on what constitutes a sound use of getters and setters. In this particular post I'm most interested in finding a good way of using getters and setters in a memory-safe way.

I hacked up a little example. Consider this epic tale of a king and his magic sword:

#include <iostream>
#include <string>

using std::string;

class Weapon {
  private:
    string name;
  public: 
    Weapon(string n = "rusty sword") : name(n) {
      std::cout << "weapon '" << name << "' constructed" << std::endl;
    }
    string getName(){
      return name;
    }
    ~Weapon(){
      std::cout << "weapon '" << name << "' destroyed" << std::endl;
    }
};

class Hero {
  private:
    string name;
    Weapon weapon;
  public:
    Hero(string n) : name(n) {
      std::cout << "hero '" << name << "' constructed" << std::endl;
      describeWeapon();
    }
    void describeWeapon() {
      std::cout << name << " now swings " << weapon.getName() << std::endl;
    }
    void setWeapon(Weapon w) {
      weapon = w;
      describeWeapon();
    }
    ~Hero() {
      std::cout << "hero '" << name << "' destroyed" << std::endl;
    }
};


int main() {
  Hero h = Hero("Arthur");
  Weapon ex = Weapon("Excalibur");
  h.setWeapon(ex);
  std::cout << "Ok, now all objects will go out of scope" << std::endl; 
  return 0;
}

This code yields the following output:

// our hero is given a rather modest sword to start with
weapon 'rusty sword' constructed    
hero 'Arthur' constructed
Arthur now swings rusty sword

// Excalibur is created in the main scope
weapon 'Excalibur' constructed
Arthur now swings Excalibur   // since we passed the object by value, Arthur now actually owns a COPY of the original Excalibur.
weapon 'Excalibur' destroyed  // This is strange! Who got destroyed here?


Ok, now all objects will go out of scope
weapon 'Excalibur' destroyed  // Here, Arthurs copy is being destroyed.
hero 'Arthur' destroyed       // Arthur himself bites the dust. 
weapon 'Excalibur' destroyed  // And finally, the original sword leaves this world.

Now, there are a few obvious problems in this piece of code:

• For one, it is not ideal that we have the original Excalibur in the main-scope, while Arthur is only passed a copy. But what would be a better solution? Passing a pointer to Excalibur comes to mind, but is that the right way to go here?
• It also seems like just overwriting Arthur's old weapon with the new one passed in the setter causes some erratic behaviour. Consider the last line in the second paragraph of the output: here, allegedly Excalibur gets destroyed, while I believe that it is much rather the rusty old sword that Arthur owned originally that gets demolished.

The vast majority of literature about getters and setters deals with primitives, so any help on best practices for passing objects (or pointers or references to objects) into setters would be highly appreciated!

Answer 1

This is a design issue, and there's no single answer.

We need to consider what's important about a Weapon. Is it value-like, or is its identity important? The real-world analogy is, "are copies all equivalent?". The answer to this lies partly in the type, but also in how we intend to use it.

In the real world, Excalibur has an identity. You can't copy it, but if you could, then each copy would have its own identity, and be owned by one person (let's ignore shared ownership, and losing it in the lake). The same couldn't be said for the name (or the colour) of Excalibur - you can write the name as many times as you like, and they are all equal. If you rename Excalibur to "a light-sabre", it doesn't change all the places you've written "Excalibur".

In cases where object identity is important, it's normal to delete the copy constructors and assignment operators:

class Weapon {
    // not copyable
    Weapon(const Weapon&) = delete; // also removes Weapon(Weapon&&)
    Weapon& operator=(const Weapon&) = delete;

    // instance data
    std::string name;

public: 
    Weapon(std::string n = "rusty sword");
    ~Weapon();

    std::string getName();
};

Now we can't copy a Weapon. We might be able to create another like it, but the two would be distinct.

Our hero needs to be assigned a Weapon. Let's assume that giving Excalibur to Arthur means that he owns it, until it's destroyed or he gives it to someone else:

#include <memory>
class Hero {
    std::unique_ptr<Weapon> weapon;

public:
    void describeWeapon() {
      std::cout << name << " now swings " << weapon->getName() << std::endl;
    }
    void setWeapon(std::unique_ptr<Weapon>&& w) {
      weapon = std::move(w);
      describeWeapon();
    }
    ~Hero();
};

If the ownership is shared (perhaps it also belongs to a pool of weapons that secretly communicate with each other), then we'll want to use std::shared_ptr<Weapon> instead of a unique pointer.

---

One thing we need to be careful of is weapon->getName() in the code above. If we're using pointers rather than objects or references, we'll need to be aware that we could be passed a null pointer. We can specify that callers must not pass us null pointers, but calling code is notorious for forgetting such contracts. An alternative is the Null Object pattern, where we create a no-effect weapon to stand in place of the null pointer.

We can arrange that like this:

class Hero {
    std::string name;
    const std::shared_ptr<Weapon> no_weapon
        = std::make_shared<Weapon>(name + "'s bare hands");
    std::shared_ptr<Weapon> weapon = no_weapon;

public:
    Hero(std::string n)
        : name(n)
    {
        std::cout << "hero '" << name << "' constructed" << std::endl;
        describeWeapon();
    }

    void describeWeapon()
    {
        std::cout << name << " now swings " << weapon->getName() << std::endl;
    }

    void setWeapon(const std::shared_ptr<Weapon>& w)
    {
        weapon = w ? w : no_weapon;
        describeWeapon();
    }
    ~Hero()
    {
        std::cout << "hero '" << name << "' destroyed" << std::endl;
    }
};

---

A further consideration, that's specific to C++: the constructors that take a single argument are candidates for conversions. You could have written:

Hero h = "Arthur";
h.setWeapon("The Holy Hand Grenade of Antioch");

and those strings would be converted to new objects. We don't want that to happen, so we should probably declare the constructors explicit.

Answer 2

@Toby Speight describes how it should probably be done.

But I want to describe what is happening in your case so that you understand the concept of copy construction and assignment.

The problem is that there are a couple of methods that are automatically generated by the compiler that you did not account for.

If you don't specify a copy constructor and assignment operator then the compiler will generate these for you. There are several places where you use them so you don't see the extra constructions. So if we manually add these to your Weapon class with some print statements then the creation and destruction will become more obvious:

The Code

#include <iostream>
#include <string>

using std::string;

class Weapon {
  public:
    // Your other code as before.

    // Add a copy constructor that prints.
    // It changes the name slightly so we can follow it.
    // As it is defined by the user the compiler does not need to generate it.
    Weapon(Weapon const& copy)
        : name(copy.name + "C")
    {
        std::cout << "Copy Construct Weapon: " << name << "\n";
    }
    // Add a copy assignment that prints.
    // It changes the name slightly so we can follow it.
    // As it is defined by the user the compiler does not need to generate it.
    Weapon& operator=(Weapon const& copy)
    {
        name = copy.name + "A";
        std::cout << "Copy Assigned Weapon: " << name << "\n";
        return *this;
    }
};

So when we run the code we get:

// Hero h = Hero("Arthur");
weapon 'rusty sword' constructed     // member weapon constructed by default constructor
hero 'Arthur' constructed            // Body of hero executed.
Arthur now swings rusty sword        // Call to describeWeapon()

// Weapon ex = Weapon("Excalibur");
weapon 'Excalibur' constructed

// h.setWeapon(ex);
Copy Construct Weapon: ExcaliburC    // setWeapon(Weapon w) Pass by value so copy made and placed in `w` 
Copy Assigned Weapon: ExcaliburCA    // weapon = w          Call the assignment operator to copy over the old sword: x
Arthur now swings ExcaliburCA        // describeWeapon();
weapon 'ExcaliburC' destroyed        // the method setWeapon() is exiting destroying the parameter w


Ok, now all objects will go out of scope
weapon 'Excalibur' destroyed        // The object `ex` goes out of scope.
hero 'Arthur' destroyed             // The object `h` is going out of scope.
weapon 'ExcaliburCA' destroyed      // The object `h.weapon` is destroyed as part of `h`