# Weapon and Hero classes

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!

• It's not so much a matter of primitive/non-primitive, but value objects and identity objects. Excalibur is a one-off, and not easily copied, so we probably ought to accept references (or shared pointers, depending on our ownership model). We can all have copies of Excalibur's name, which is a (non-primitive) std::string. It's appropriate to copy the name by value (if one of the names is destroyed, the others are unhurt). – Toby Speight Dec 14 '17 at 17:25
• It's instructive to provide some instrumented copy constructors and assignment operators for this example - that would answer the question "*strange! Who got destroyed here?". – Toby Speight Dec 14 '17 at 17:28
• @TobySpeight: Thank you very much for the help! Glad to see that a Scotsman would help out with a question on Excalibur :) You're right, I need to decide whether I should only pass the name of the sword by value or a pointer/ref to the whole object. In the given example I've been mixing Weapons being constructed by the Hero with Weapons being constructed outside - that was inconsistent. I'll provide examples of both ways tomorrow if nobody posts a nicer example. Thanks! – Michael Langbein Dec 14 '17 at 17:47
• @TobySpeight: as for instrumented assignment and constructors: I have to admit that this is the first time that I heard about them. I found a nice source about the subject and will try to incorporate them in the code! – Michael Langbein Dec 14 '17 at 17:49
• I just meant instrumented in the same way that you instrumented the std::string constructor and the destructor! BTW, I'm only in Scotland by choice, not by birth... – Toby Speight Dec 14 '17 at 18:08

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.

• I couldn't have hoped for a more helpful response. I'll need some time to learn more about unique pointers as well as operator overloading. But most of all thanks for the concept of non-copyable instances. That will give me something to work with! – Michael Langbein Dec 14 '17 at 20:26

@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

• I heard about operator overloading for the first time today. Will probably be using this as a debugging tool in all future projects from now on :) Thanks! – Michael Langbein Dec 14 '17 at 20:30
• @MichaelLangbein Operator overloading is something slightly different. It is also generally considered bad practice (though there are few special cases were it works (were you are defining an arithmetic type)). This is "method overloading" but more specifically "constructor overloading" or you could look at automatic compiler generated methods. – Martin York Dec 14 '17 at 20:54