Memento Pattern failing because of pointer data members

All the undos and redos were working fine until the pointer data members came into the picture. The problem is that the values pointed to changed, but the pointers themselves did not. So restoring gives the same pointers but they still point to the new values. I think the solution is that the classes that are the pointer data members themselves need their own Mementos (which would be a lot of work because there are many data member pointers in my program). Is that the only approach? I just need a yes or no answer before I proceed with that idea. Trying to rewrite the assignment operator failed to fix the problem.
Here is a sample code to show what I'm talking about. You can compile and run the program to see the problem it has restoring values of pointer data members:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
#include <iostream>
#include <string>
#include <memory>
#include <vector>
#include <list>
#include <cstdlib>
#include <ctime>

class FoodHistory {
	public:
		int numMeals = 0;  // plus many other data members
};

class Animal {
    public:
        int i = 0;
        double d = 0;
   		FoodHistory* foodHistory = new FoodHistory();
	public:
        class Memento {
        	private:
            	std::unique_ptr<Animal> copy;
            public:
	            Memento(): copy (nullptr) {}
	            Memento (const Animal* being): copy (being->clone()) {}
				friend class Animal;
	    };
        virtual ~Animal() noexcept = default;
        Animal& operator = (const Animal& other) {
        	if (&other == this) return *this;
        	i = other.i;  d = other.d;  foodHistory = other.foodHistory;
			*foodHistory = *other.foodHistory;  // Attempt at debugging fails
        	return *this;
        }
	    virtual void change() = 0;
	    virtual void display() const = 0;
	    virtual Memento createMemento() const {return Memento (this);}
	    virtual Animal& restoreMemento (const Memento& memento) {return assign (*memento.copy);}
	private:
	    virtual std::unique_ptr<Animal> clone() const = 0;
        virtual Animal& assign (const Animal&) = 0 ;
};

template <typename DERIVED>
class AnimalCRTP: public Animal {
    protected:
        virtual std::unique_ptr<Animal> clone() const override {
        	return std::unique_ptr<Animal>(new DERIVED (static_cast<const DERIVED&>(*this)));
		}
        virtual Animal& assign (const Animal& being) override {  // return Animal& because may throw std::bad_cast
        	return static_cast<DERIVED&>(*this) = dynamic_cast<const DERIVED&>(being);
		}
};

class Dog: public AnimalCRTP<Dog> {
	public:
	    virtual void change() override {i++; d += (float)(std::rand() % 10) / 10;  foodHistory->numMeals++;}
	    virtual void display() const override {std::cout << "Dog {" << i << ", " << d << "}, foodHistory->numMeals = " << foodHistory->numMeals<< std::endl;}
};

class Cat: public AnimalCRTP<Cat> {
	public:
	    virtual void change() override {i += 10; d += (float)(std::rand() % 10) / 10;  foodHistory->numMeals++;}
    	virtual void display() const override {std::cout << "Cat {" << i << ", " << d << "}, foodHistory->numMeals = " << foodHistory->numMeals<< std::endl;}
};

class Command {	 // The rest of the code is just for running the programming
  	private:
  		typedef void (Animal::*Action)();
	    std::vector<Animal*> receivers;
//	    std::vector<FoodHistory*> foodHistories;  // these need their own Mementos perhaps???
	    Action action;
	    static int numReceivers;
	    static std::vector<std::vector<Command*>> commandList;
	    static std::vector<std::vector<Animal::Memento>> mementoList;
	    static std::size_t numCommands;
	    static std::size_t maxCommands;
	public:
	    Command (const std::vector<Animal*>& newReceivers, Action newAction): receivers (newReceivers), action (newAction) {
			numReceivers = receivers.size();
		}
	    virtual void execute() {
	    	if (mementoList.size() < numCommands + 1)
	    	{
	    		mementoList.resize (numCommands + 1);
	    		mementoList[numCommands].resize (numReceivers);
	    	}
	        for (int i = 0; i < numReceivers; ++i)
				mementoList[numCommands][i] = receivers[i]->createMemento();
	    	if (commandList.size() < numCommands + 1)
	    	{
	    		commandList.resize (numCommands + 1);
	    		commandList[numCommands].resize (numReceivers);
	    	}
			for (int i = 0; i < numReceivers; ++i)
	        	commandList[numCommands][i] = this;
	        if (numCommands > maxCommands)
	          	maxCommands = numCommands;
	        numCommands++;
	        for (Animal* x: receivers)
	        	(x->*action)();
	    	if (mementoList.size() < numCommands + 1)
	    	{
	    		mementoList.resize (numCommands + 1);
	    		mementoList[numCommands].resize (numReceivers);
	    	}
	    	if (commandList.size() < numCommands + 1)
	    	{
	    		commandList.resize (numCommands + 1);
	    		commandList[numCommands].resize (numReceivers);
	    	}
	    	for (int i = 0; i < numReceivers; ++i)
	    	{
	        	mementoList[numCommands][i] = receivers[i]->createMemento();
	        	commandList[numCommands][i] = this;
	        }
	    }
	    static void undo() {
	        if (numCommands == 0)
	        {
	            std::cout << std::endl << "[There is nothing to undo at this point.]" << std::endl << std::endl;
	            return;
	        }
	        for (int i = 0; i < numReceivers; ++i)
	        	commandList[numCommands - 1][i]->receivers[i]->restoreMemento (mementoList[numCommands - 1][i]);
	        numCommands--;
	    }
	    void static redo() {
	        if (numCommands > maxCommands)
	        {
	            std::cout << std::endl << "[There is nothing to redo at this point.]" << std::endl << std::endl;
	            return ;
	        }
			for (int i = 0; i < numReceivers; ++i)
	        	commandList[numCommands + 1][i]->receivers[i]->restoreMemento (mementoList[numCommands + 1][i]);
	        numCommands++;
	    }
};

int Command::numReceivers;
std::vector<std::vector<Command*>> Command::commandList;
std::vector<std::vector<Animal::Memento>> Command::mementoList;
std::size_t Command::numCommands = 0;
std::size_t Command::maxCommands = 0;

int main() {
	srand (std::time (nullptr));
	int i;
	std::vector<Animal*> pets = {new Dog, new Cat};
	
	Command *commands[2];
	commands[1] = new Command (pets, &Animal::change);  // add whatever other types of commands from Animal functions
	
	for (const Animal* x: pets)
		x->display();
	std::cout << std::endl << "0.Exit,  1.Change,  2.Undo,  3.Redo: ";
	std::cin >> i;
	
	while (i != 0)
	{
		if (i < 0 || i > 3)
		{
			std::cout << "Enter a proper choice: ";
			std::cin >> i;
			continue;
		}			
		else if (i == 2)
		  	Command::undo();
		else if (i == 3)
		  	Command::redo();
		else
		  	commands[i]->execute();
		for (const Animal* x: pets)
			x->display();
		std::cout << std::endl << "0.Exit,  1.Change,  2.Undo,  3.Redo: ";
		std::cin >> i;
	}
} 
Last edited on
> FoodHistory* foodHistory = new FoodHistory();
I'm going to assume that you do have a good reason to use a pointer there.

Your destructor is incorrect, you'll leak memory.
You are missing a proper copy constructor too.


Looking at your assignment operator
1
2
3
4
5
6
7
8
        Animal& operator = (const Animal& other) {
        	if (&other == this) return *this;
        	i = other.i;  
        	d = other.d;  
        	foodHistory = other.foodHistory;
        	*foodHistory = *other.foodHistory;  //A = A
        	return *this;
        }
when you assign one Animal to other, their history becomes shared ¿was that your intention?

If you want to hold a copy then it should be
1
2
delete foodHistory;
foodHistory = other.foodHistory->clone();
(again, assuming that you do need a pointer there)
So all the classes that are pointer data members in Animal (which are big classes, hence the need for them to be pointer data members) only need their own clone function, and do not need their own Memento class? That would be a relief, if it is true.

Update: It is working now. Thanks! Just for those who wanted to know, the fix was

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
class FoodHistory {
	public:
		int numMeals = 0;
	        FoodHistory* clone() const {return new FoodHistory (*this);}
};

class Animal {
//....
        Animal() = default;
        Animal(const Animal& other): i (other.i), d (other.d) {
			delete foodHistory;
			foodHistory = other.foodHistory->clone();
		}
        Animal& operator = (const Animal& other) {
        	if (&other == this) return *this;
        	i = other.i;  d = other.d;
			delete foodHistory;
			foodHistory = other.foodHistory->clone();
        	return *this;
        }
// ...
};


I hope that's what ne555 meant. Is it ideal? We'll it works at least.
Last edited on
Topic archived. No new replies allowed.