2
\$\begingroup\$

Here I illustrate the use of the Memento Pattern to restore certain (but not all) data of a class that has data in abstract base classes. A Spellcaster has spells and is derived from an abstract CharacterClass that has levels, etc... data, which in turn is derived from LivingBeing which has hitPoints, etc... data, and all of these are to be restored to their previous values in previous levels. But name and some other data in LivingBeing shall not be restored, nor should wandsPossessed in Spellcaster. A vector of LevelMementos is stored throughout the hierarchy to restore the data in each of these classes. The data that is to restored is stored in another inner class called Data, and so the inner class LevelMemento uses the inner class Data in its constructor. All other data that is not to be restored are placed outside the inner class Data. The methods save(std::ostream&) and load(std::istream&) are not needed in my code, but I wanted to test them out and illustrate that the Data inner classes also help to implemente them without needing to rewrite any duplicate lines when the LevelMementos are written and read from a text file.

#include <iostream>
#include <memory>
#include <vector>
#include <list>
#include <array>
#include <fstream>

enum Ability { Strength, Intelligence, Wisdom, Dexterity, Constitution, Charisma, NumAbilityScores, FirstAbilityIndex = Strength, LastAbilityIndex = Charisma };

std::list<class LivingBeing*> allBeingsPresent;

class LivingBeing {
    struct Data {  // Data stores all the LivingBeing data that shall be restored to what they were in previous levels after energy drains.
        int hitPoints, numAttacks = 1;
        void save (std::ostream& os) const {
            os << hitPoints << ' ' << numAttacks << '\n';
        }
        void load (std::istream& is) {
            is >> std::skipws >> hitPoints >> numAttacks;
        }
    };
    struct LevelMemento {
        Data data;
        LevelMemento (const Data& d) : data(d) { }
        LevelMemento() = default;
        void save (std::ostream& os) const { data.save(os); }  // Thanks to the Data struct, this needs not be rewritten.
        void load (std::istream& is) { data.load(is); }  // Thanks to the Data struct, this needs not be rewritten.
    };
    Data data;
    std::string name;  // name will NOT be restored to what it was in previous levels after energy drains, nor will AC, encumbrance, etc... below.
    int AC = 9, encumbrance = 800;
    std::vector<std::unique_ptr<LevelMemento>> levelMementos;
public:
    LivingBeing (const std::string& n, int hp) : name(n) { data.hitPoints = hp;  allBeingsPresent.push_back(this); }
    LivingBeing() = default;
    std::string getName() const { return name; }
    void changeName (const std::string& newName) { name = newName; }
    virtual void levelUp() {
        std::cout << '\n' << name << " has levelled up.\n";
        levelMementos.push_back(std::unique_ptr<LevelMemento>(new LevelMemento(data)));  // Add a snapshot of the current value of 'data' to levelMementos.
        data.hitPoints += 5;
    }
    virtual void revertToPreviousLevel (int previousLevel) {
        data = levelMementos[previousLevel - 1]->data;
        levelMementos.resize(previousLevel);   // Remove all elements of levelMementos after index previousLevel - 1.
    }
    virtual void receiveEnergyDrain (int numLevels = 1) = 0;  // Not only does CharacterClass override this, but so does Monster (its hitDice will represent its level).
    virtual std::string saveName() const = 0;  // LivingBeing is abstract, but LivingBeing::data will still be restored via levelMementos.
    virtual void save (std::ostream& os) const {
        os << saveName() << '\n';
        data.save(os);
        os << name << '\n';
        os << AC << ' ' << encumbrance << '\n';
        os << levelMementos.size() << '\n';
        for (const std::unique_ptr<LevelMemento>& memento : levelMementos)
            memento->save(os);
    }
    virtual void load (std::istream& is) {
        std::string name;
        int numMementos;
        data.load(is);
        while (std::getline(is, name) && name.empty());
        is >> std::skipws >> AC >> encumbrance;
        is >> numMementos;
        for (int i = 0;  i < numMementos;  ++i) {
            std::unique_ptr<LevelMemento> memento = std::make_unique<LevelMemento>();
            memento->load(is);
            levelMementos.push_back(std::move(memento));
        }
    }
    virtual void display() const {
        std::cout << "Name: " << name << '\n';
        std::cout << "Hit points: " << data.hitPoints << '\n';
    }
};

class CharacterClass : public LivingBeing {
    struct Data {  // Data stores all the CharacterClass data that shall be restored to what they were in previous levels after energy drains.  CharacterClass is abstract, but CharacterClass::data will still be restored via CharacterClass::levelMementos.
        int level = 1, experiencePoints = 0;
        std::array<int, NumAbilityScores> abilities = {{10,14,10,15,12,9}};
        void save (std::ostream& os) const {
            os << level << ' ' << experiencePoints << '\n';
            for (int n : abilities)
                os << ' ' << n;
            os << '\n';
        }
        void load (std::istream& is) {
            is >> std::skipws >> level >> experiencePoints;
            for (int i = 0;  i < NumAbilityScores;  ++i)
                is >> std::skipws >> abilities[i-1];
        }
    };
    struct LevelMemento {
        Data data;
        LevelMemento (const Data& d) : data(d) { }
        LevelMemento() = default;
        void save (std::ostream& os) const { data.save(os); }  // Thanks to the Data struct, this needs not be rewritten.
        void load (std::istream& is) { data.load(is); }  // Thanks to the Data struct, this needs not be rewritten.
    };
    Data data;
    std::vector<std::unique_ptr<LevelMemento>> levelMementos;
public:
    using LivingBeing::LivingBeing;
    int getAbilityScore (Ability a) const { return data.abilities[a]; }
    void setAbilityScore (Ability a, int value) { data.abilities[a] = value; }
    virtual void levelUp() {
        levelMementos.push_back(std::unique_ptr<LevelMemento>(new LevelMemento(data)));  // Add a snapshot of the current value of CharacterClass::data to CharacterClass::levelMementos.
        LivingBeing::levelUp();  // Add a snapshot of LivingBeing::data to LivingBeing::levelMementos.
        data.level++;
        data.experiencePoints += 5000;
    }
    virtual void revertToPreviousLevel (int previousLevel) override {
        LivingBeing::revertToPreviousLevel(previousLevel);  // Restores the data in LivingBeing that is not in CharacterClass (i.e. hitPoints, numAttacks, etc...).
        data = levelMementos[previousLevel - 1]->data;
        levelMementos.resize(previousLevel);   // Remove all elements of levelMementos after index previousLevel - 1.
    }
    virtual void receiveEnergyDrain (int numLevels = 1) override {
        std::cout << '\n' << getName() << " has been energy drained.  Number of levels lost: " << numLevels << '\n';
        revertToPreviousLevel(data.level - numLevels);
    }
    virtual void save (std::ostream& os) const override {
        LivingBeing::save(os);
        saveParticular(os);
    }
    virtual void load (std::istream& is) override {
        LivingBeing::load(is);
        loadParticular(is);
    }
    virtual void display() const {
        LivingBeing::display();
        for (int a : data.abilities)
            std::cout << a << ' ';
        std::cout << "\nLevel: " << data.level << '\n';
        std::cout << "ExperiencePoints: " << data.experiencePoints << '\n';
    }
private:
    void saveParticular (std::ostream& os) const {
        data.save(os);
        os << levelMementos.size() << '\n';
        for (const std::unique_ptr<LevelMemento>& memento : levelMementos)
            memento->save(os);
    }
    void loadParticular (std::istream& is) {
        int numMementos;
        data.load(is);
        is >> numMementos;
        for (int i = 0;  i < numMementos;  ++i) {
            std::unique_ptr<LevelMemento> memento = std::make_unique<LevelMemento>();
            memento->load(is);
            levelMementos.push_back(std::move(memento));
        }
    }
};

struct Spell {
    void save (std::ostream&) const { }  // Implementation omitted.
    void load (std::istream&) { }  // Implementation omitted.
};

struct Wand {
    void save (std::ostream&) const { }  // Implementation omitted.
    void load (std::istream&) { }  // Implementation omitted.
};

class Spellcaster : public CharacterClass {
    struct Data {  // Spellcaster::Data stores all the Spellcaster data that shall be restored to what they were in previous levels after energy drains.
        std::list<std::shared_ptr<Spell>> fullListOfSpells;
        void save (std::ostream& os) const {
            os << fullListOfSpells.size() << '\n';
            for (const std::shared_ptr<Spell>& spell : fullListOfSpells)
                spell->save(os);
        }
        void load (std::istream& is) {
            int numSpells;
            is >> numSpells;
            for (int i = 0;  i < numSpells;  ++i) {
                std::shared_ptr<Spell> spell = std::make_shared<Spell>();
                spell->load(is);
                fullListOfSpells.push_back(spell);
            }
        }
    };
    struct LevelMemento {
        Data data;
        LevelMemento (const Data& d) : data(d) { }
        LevelMemento() = default;
        void save (std::ostream& os) const { data.save(os); }  // Thanks to the Data struct, this needs not be rewritten.
        void load (std::istream& is) { data.load(is); }  // Thanks to the Data struct, this needs not be rewritten.
    };
    Data data;
    std::list<std::shared_ptr<Spell>> currentListOfSpells;  // currentListOfSpells will NOT be restored to what it was in previous levels after energy drains, nor will wandsPossessed, etc... below.
    std::list<Wand*> wandsPossessed;
    std::vector<std::unique_ptr<LevelMemento>> levelMementos;
    static const std::string tag;
    friend class System;
public:
    Spellcaster (const std::string& name, int hitPoints, const std::list<std::shared_ptr<Spell>>& spells) : CharacterClass(name, hitPoints), data({spells}), currentListOfSpells(spells) { }
    Spellcaster() = default;
    void addWand (Wand* wand) { wandsPossessed.push_back(wand); }
    virtual void revertToPreviousLevel (int previousLevel) override {
        CharacterClass::revertToPreviousLevel(previousLevel);  // Restores the data in CharacterClass that is not in Spellcaster (i.e. abilities, level, experiencePoints, etc...).
        data = levelMementos[previousLevel - 1]->data;  // Restores the data in Spellcaster specifically (i.e. fullListOfSpells).
        levelMementos.resize(previousLevel);   // Remove all elements of levelMementos after index previousLevel - 1.
    }
    virtual void levelUp() override {
        levelMementos.push_back(std::unique_ptr<LevelMemento>(new LevelMemento(data)));  // Add a snapshot of the current value of Spellcaster::data to Spellcaster::levelMementos.
        CharacterClass::levelUp();  // Add a snapshot of CharacterClass::data to LivingBeing::levelMementos.
        std::shared_ptr<Spell> spell = std::make_shared<Spell>();
        data.fullListOfSpells.push_back(spell);
        currentListOfSpells.push_back(spell);
    };
    virtual void save (std::ostream& os) const override {
        CharacterClass::save(os);
        saveParticular(os);
    }
    virtual void load (std::istream& is) override {
        CharacterClass::load(is);
        loadParticular(is);
    }
    virtual void display() const override {
        CharacterClass::display();
        std::cout << "Full list of spells known: " << data.fullListOfSpells.size() << '\n';
        std::cout << "Number of wands: " << wandsPossessed.size() << '\n';
    }
private:
    virtual std::string saveName() const override { return tag; }
    void saveParticular (std::ostream& os) const {
        data.save(os);
        os << currentListOfSpells.size() << '\n';
        for (const std::shared_ptr<Spell>& spell : currentListOfSpells)
            spell->save(os);
        os << wandsPossessed.size() << '\n';
        for (const Wand* wand : wandsPossessed)
            wand->save(os);
        os << levelMementos.size() << '\n';
        for (const std::unique_ptr<LevelMemento>& memento : levelMementos)
            memento->save(os);
    }
    void loadParticular (std::istream& is) {
        data.load(is);
        int num;
        is >> num;
        for (int i = 0;  i < num;  ++i) {
            std::shared_ptr<Spell> spell = std::make_shared<Spell>();
            spell->load(is);
            currentListOfSpells.push_back(spell);
        }
        is >> num;
        for (int i = 0;  i < num;  ++i) {
            Wand* wand = new Wand;
            wand->load(is);
            wandsPossessed.push_back(wand);
        }
        is >> num;
        for (int i = 0;  i < num;  ++i) {
            std::unique_ptr<LevelMemento> memento = std::make_unique<LevelMemento>();
            memento->load(is);
            levelMementos.push_back(std::move(memento));
        }
    }
};
const std::string Spellcaster::tag = "Spellcaster";

struct System {
    static const std::string saveFileName;
    static void save();
    static void load();
};
const std::string System::saveFileName = "SaveFile.txt";

void System::save() {
    std::ofstream outfile(saveFileName);
    outfile << allBeingsPresent.size() << '\n';
    for (const LivingBeing* being : allBeingsPresent)
        being->save(outfile);
}

void System::load() {
    std::ifstream infile(saveFileName);
    int numBeings;
    std::string type;
    infile >> numBeings;
    for (int i = 0;  i < numBeings;  ++i) {
        while (std::getline(infile, type) && type.empty());
        if (type == Spellcaster::tag) {
            Spellcaster* p = new Spellcaster;
            p->load(infile);
            allBeingsPresent.push_back(p);
        }
//      else if etc...
    }
}

int main() {
    Spellcaster* merlin = new Spellcaster("Merlin", 5, { std::make_shared<Spell>() } );
    merlin->display();
    merlin->levelUp();
    merlin->display();
    merlin->levelUp();
    merlin->display();
    merlin->changeName("Merlin the Magic-User");
    merlin->addWand(new Wand);  // Note that the wands possessed will NOT be restored to what they were during previous levels after energy drains.
    merlin->setAbilityScore(Intelligence, 18);
    std::cout << "\nMerlin has changed his name to " << merlin->getName() << " after he found a wand, and his intelligence changed to " << merlin->getAbilityScore(Intelligence) << ".\n";
          // Note that his name will NOT be restored to what it was during previous levels after energy drains, but intelligence SHALL be restored.
    merlin->levelUp();
    merlin->display();
    
    merlin->receiveEnergyDrain();
    merlin->display();
    merlin->receiveEnergyDrain(2);
    merlin->display();
}

Output:

Name: Merlin
Hit points: 5
10 14 10 15 12 9
Level: 1
ExperiencePoints: 0
Full list of spells known: 1
Number of wands: 0

Merlin has levelled up.
Name: Merlin
Hit points: 10
10 14 10 15 12 9
Level: 2
ExperiencePoints: 5000
Full list of spells known: 2
Number of wands: 0

Merlin has levelled up.
Name: Merlin
Hit points: 15
10 14 10 15 12 9
Level: 3
ExperiencePoints: 10000
Full list of spells known: 3
Number of wands: 0

Merlin has changed his name to Merlin the Magic-User after he found a wand, and his intelligence changed to 18.

Merlin the Magic-User has levelled up.
Name: Merlin the Magic-User
Hit points: 20
10 18 10 15 12 9
Level: 4
ExperiencePoints: 15000
Full list of spells known: 4
Number of wands: 1

Merlin the Magic-User has been energy drained.  Number of levels lost: 1
Name: Merlin the Magic-User
Hit points: 15
10 18 10 15 12 9
Level: 3
ExperiencePoints: 10000
Full list of spells known: 3
Number of wands: 1

Merlin the Magic-User has been energy drained.  Number of levels lost: 2
Name: Merlin the Magic-User
Hit points: 5
10 14 10 15 12 9
Level: 1
ExperiencePoints: 0
Full list of spells known: 1
Number of wands: 1

Update: Since this posting, I've rewritten the code so that LevelMemento has been defined outside the classes and now has a template parameter T for the class it is being used in. So now the LevelMemento<T> class is written out only once. I'm looking for more ways to shorten the code.

\$\endgroup\$
1
  • \$\begingroup\$ Please edit your question so that the title describes the purpose of the code, rather than its mechanism. We really need to understand the motivational context to give good reviews. Thanks! \$\endgroup\$ Nov 27, 2022 at 9:03

2 Answers 2

5
\$\begingroup\$

Unnecessary use of smart pointers

There is no need to use std::unique_ptr for LevelMementos. Consider that std::vector is already managing memory for you, and there is nothing special about LevelMemento that would prevent it from being usable directly in a std::vector (like being non-copyable). So just write:

std::vector<LevelMemento> levelMementos;

To load them in you can write:

for (int i = 0; i < numMementos; ++i) {
    LevelMemento memento;
    memento.load(is);
    levelMementos.push_back(memento);
}

Now you are copying a whole LevelMemento instead of just a pointer to it, but the compiler will probably optimize the copies away. You can avoid having to copy entirely by writing something like:

for (int i = 0; i < numMementos; ++i) {
    levelMementos.emplace_back();
    levelMementos.back().load(is);
}

However, ideally you would add a constructor to LevelMemento that takes a std::istream&, so you can write:

for (int i = 0; i < numMementos; ++i) {
    levelMementos.emplace_back(is);
}

I cannot see the implementation of Spell, but I suspect you also don't need to use std::shared_ptr here.

Use of bare pointers

While you used smart pointers for levels and spells, I see that allBeingsPresent and wandsPossessed are lists of bare pointers. I see new being used, but no delete, so those are memory leaks. Either store by value (which is preferrable), or if that is not possible, use a smart pointer.

In the case of LivingBeing, that is a base class, so then you have to store by pointer. In that case, use std::unique_ptr:

std::list<std::unique_ptr<LivingBeing>> allBeingsPresent;

Note that you can also use std::vector here; resizing the vector will only cause the pointers being moved around, not the data pointed to itself. To add derived classes to this list, you can use std::make_unique<Derived> and push the result, like so:

allBeingsPresent.push_back(std::make_unique<Spellcaster>(infile));

In main() you also have a memory leak. There is no reason to use a pointer here. Why not just write the following?

Spellcaster merlin("Merlin", 5, {Spell{}});
merlin.display();
…

Naming things

While the software design pattern has a name, you don't have to name your data structures after that pattern. In fact, it might be confusing for someone who has not heard of the name of this pattern. Also, there is no need to make a separate struct LevelMemento, Data itself is already the memento in the memento pattern.

Data itself is a very generic name. In D&D, things like hit points and the number of attacks are part of a character's statistics, usually abbreviated as stats. So I would write:

class LivingBeing {
    struct Stats {
         int hitPoints;
         int numAttacks = 1;
         void save(std::ostream& os) const {…}
         void load(std::istream& is) {…}
    };

    Stats stats;
    std::vector<Stats> levelStats;
    …
};

Of course, AC and encumbrance might also be considered "stats", so maybe Stats is not the best name, maybe PerLevelStats or something like that would be the most precise name.

CharacterClass is also misnamed, it should be Character. If the Class part refered to it being a C++ class, then that would be redundants, and you don't use that suffix for other classes. If you meant it was a D&D character class, then that would also be wrong, as it would actually be classes like SpellCaster that implement a character class.

Don't hardcode printing to std::cout

Your display() functions always print to std::cout. But you can make it more generic by taking a std::ostream& parameter, which you could default to std::cout:

virtual void display(std::ostream& os = std::cout) const {
    os << "Name: " << name << '\n';
    …
}

Now you can print to any output stream, like a file or a std::stringstream. Alternatively, create a friend operator<<(std::ostream&) overload so instead of calling display(), you can write:

SpellCaster merlin(…);
std::cout << merlin;

Where is the caretaker?

The memento pattern involves three types of objects: the originator (your LivingBeing and its derived classes), the memento (Data in each of those classes), and the caretaker. The latter should be the code that takes care of saving and loading mementos, removing that responsibility from the originator. However, in your case the originators also fulfill the caretaker duties.

Ideally, if you want to following the memento pattern to the letter, you should have separate caretaker classes that handle (re)storing the mementos to/from the originators. It should allow you to swap out the caretaker with a different one.

That said, I don't think it's necessarily bad that your code mixes the originator and caretaker duties in the same class, after all the history of stats for each level belongs to a character, so it doesn't make sense to store that history outside of LivingBeing and its derived classes.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks for the great review. If I wish the optimize LivingBeing's copy constructor performance (e.g. with clone()), while avoiding memory leak when the vector is resized (which would happen if using a vector of raw pointers), would it be wise to use a vector of shared_ptr instead of vector of objects for the levelStats? \$\endgroup\$
    – prestokeys
    Nov 27, 2022 at 18:12
  • \$\begingroup\$ Good question. I amended my answer to be more clear about this. You would indeed use a std::list<std::unique_ptr<LivingBeing>> for allBeingsPresent. LevelStats are not derived classes, so there is no need to use pointers here as far as I can tell. Finally, if you must use pointers, then first check if it can be done with std::unique_ptr. std::shared_ptr allows copies, but there is a cost due to reference counting. Also, if you clone a LivingBeing and then change its stats, do you really want those new stats to be shared with the original? \$\endgroup\$
    – G. Sliepen
    Nov 27, 2022 at 18:47
1
\$\begingroup\$

To add,

Unscoped global enum leaks names into the global scope. Make it enum class, probably?


LivingBeing::Data::hitPoints should have default value. It is left uninitialized when a LivingBeing::LevelMemento object is default constructed. It is not obvious in particular because there are multiple declarations reside in one line.


The LivingBeing needs virtual ~LivingBeing() = default; to make the solution less error-prone. Same for children, if we implement inheritance hierarchy. Note that declaring destructor prevents auto-generation of move-specific methods. You probably need to apply the rule of 5.


There is the conventional rule to make a class definition more readable.

Regarding the matter, it is not obvious that CharacterClass is in fact abstract. We should emphasize that.


LivingBeing::Data is an aggregate class type, so we can put a corresponding object initialization into the initializer list

LivingBeing(int hp, const std::string& n) : data{hp}, name{n} { }

Data::numAttacks is left default-initialized. Note the order of arguments.


Class get-method may return a value or a reference. It looks like the LivingBeing::getName could return a reference.

const std::string& getName() const { return name; }
std::string& getName() { return name; }

We can also remove a redundant copying for rvalues,

void changeName (std::string newName) {
    name = std::move(newName);
}

It would be better to have classes defined in different files and more spaces between the lines.


Where possible, C++ ignores spaces (example), which enables us to format a printing expression according to the actual output.

os << name << '\n'
   << AC << ' ' << encumbrance << '\n'
   << levelMementos.size() << '\n';

The use of spaces inside function declarations is inconsistent, e.g.

virtual void load (std::istream& is) override {
//...
virtual void display() const {

Default arguments should not belong to pure virtual functions.


Looks like we use memento wrong. Memento represents a snapshot of an object. And a snapshot stores duplicates of values. Instead, we have some class fields declared inside the snapshot subclass. Consider example

private:
    struct State {
        int hitPoints;
        int numAttacks;
        //...
    };

    std::string name;
    int hitPoints = 0;
    int numAttacks = 1; 
    int AC = 9;
    int encumbrance = 800;
    std::vector<State> statesByLevel;

This is a minor change. But I would think about other implementation alternatives like separating Memento from an actual entity trough dependency or, may be, add a Memento kind of mixin. All to save some brain cells.

\$\endgroup\$
1
  • \$\begingroup\$ Many good points I missed in my review! \$\endgroup\$
    – G. Sliepen
    Nov 28, 2022 at 23:24

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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