C++

Forum

 
Is this the correct way to write a copy constructor for my program? Please spare a moment.

Tiger58 (117)
I am very worried...I have a project to turn in soon but my program suddenly decided to go crazy on me. Now, it crashes whenever I call a function.

I did a bit of research and it seems I may need a copy constructor and assignment operator, but it seems complicated because of the way my class is made, here is the example:

I only included the variables here
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
 
Player
{
private:
		static const int SKILL_NUM=4;
		static const int MAIN_ELEMENTAL_SKILLS=8;
		//Basic information
		string name;
		string element;
		string type;
		int level;      
		int experience;
		int totalExp;
		int gold;
		int itemCount;
		int ID;
		int statPoints;
		int currentHealth;
		int allyCount;
		//Elemental experience (from using elemental skills)
		int elementalExp[MAIN_ELEMENTAL_SKILLS];
		//Weapon experience and levels 
		int swordsmanExp;
		int swordsmanLevel;
		int clericExp;
		int clericLevel;
		int hunterExp;
		int hunterLevel;
		int barbarianExp;
		int barbarianLevel;
		//Player stats
		Stats stats;
		Stats totalStats;
		//Player skills
		double weaponCombo;
		Skills physical[SKILL_NUM];
		Skills offensive[SKILL_NUM];
		Skills defensive[SKILL_NUM];
		//Equipment slots
		Item *mainHand;
		Item *offHand;
		Item *head;
		Item *body;
		Item *feet;
		Item *ring1;
		Item *ring2;
		animArrays screen;
}


So as you can see, it contains multiple other classes. Should I make a copy constructor for them, too?

But then another issue arises that I don't know how to deal with, you see, the Item objects use polymorphism. I don't know how this affects any of it, and I have absolutely no idea as to how I would go about making the copy constructor/assignment operator for them.

Side note, I was running this class (WITHOUT a copy constructor or assingment operator) and equating Player objects freely, sorting them and such, equaling them to each other. Do you think this is why my program now crashes so suddenly?

This is what I currently have, just the basic really, I don't know how to start working out the Item objects, or how they would be affected. 
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
 
Player::Player(const Player& other)
{
	name=other.name;
	element=other.element;
	type=other.type;
	level=other.level;
	experience=other.experience;
	gold=other.gold;
	itemCount=other.itemCount;
	ID=other.ID;
	statPoints=other.statPoints;
	totalExp=other.totalExp;
	allyCount=other.allyCount;
	weaponCombo=other.weaponCombo;
	currentHealth=other.currentHealth;
	
	for(int i=0;i<MAIN_ELEMENTAL_SKILLS;i++)
		elementalExp[i]=other.elementalExp[i];

	for(int i=0;i<4;i++)
	{
		physical[i]=other.physical[i];
		offensive[i]=other.offensive[i];
		defensive[i]=other.defensive[i];
	}

	swordsmanExp=other.swordsmanExp;
	swordsmanLevel=other.swordsmanLevel;
	clericExp=other.clericExp;
	clericLevel=other.clericLevel;
	hunterExp=other.hunterExp;
	hunterLevel=other.hunterLevel;
	barbarianExp=other.barbarianExp;
	barbarianLevel=other.barbarianLevel;


}
Last edited on
codewalker (394)
if Stats, Skills and Item are classes/structs by themselves then you need to define copy constructors/copy assignment operators for them as well

Then for Item * you need to do new on it for example mainHand = new Item(*(other.mainHand));
JLBorges (13770)
You are trying to do way too much in one class. Consider splitting it up into a set of collaborating classes.

Most of the time, you should be writing foundation operations only for low level shim classes that encapsulate the management of a single resource. Normally, let the implementation decide on the semantics of copy and move, and synthesize implicitly declared copy constructors etc.

For instance, in the class Player, make sure that the members are copyable:
for instance, animArrays is a std::vector<>; Stats, Skills are copyable types, the pointers are some kind of (copyable) smart pointers (make sure that Item has a virtual destructor).

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
 
struct Player
{
private:
		static const int SKILL_NUM=4;
		static const int MAIN_ELEMENTAL_SKILLS=8;
		//Basic information
		std::string name;
		std::string element;
		std::string type;
		int level;
		int experience;
		int totalExp;
		int gold;
		int itemCount;
		int ID;
		int statPoints;
		int currentHealth;
		int allyCount;
		//Elemental experience (from using elemental skills)
		int elementalExp[MAIN_ELEMENTAL_SKILLS];
		//Weapon experience and levels
		int swordsmanExp;
		int swordsmanLevel;
		int clericExp;
		int clericLevel;
		int hunterExp;
		int hunterLevel;
		int barbarianExp;
		int barbarianLevel;

		//Player stats
		Stats stats;
		Stats totalStats;
		//Player skills
		double weaponCombo;
		Skills physical[SKILL_NUM];
		Skills offensive[SKILL_NUM];
		Skills defensive[SKILL_NUM];

		//Equipment slots
		using item_pointer = std::shared_ptr<Item> ;
		item_pointer mainHand;
		item_pointer offHand;
		item_pointer head;
		item_pointer body;
		item_pointer feet;
		item_pointer ring1;
		item_pointer ring2;
		animArrays screen; // animArrays is a std::vector<>
};


And then let the implementation synthesize the copy constructor and assignment operator.
Tiger58 (117)
I will take the advice, but, question...I made the copy constructors and assignment operator. Yet, the program still crashes. Is there something I need to do? I thought the issue was temporary, and that it would go away once I did that. It is frustrating, since I cannot see where the problem lies exactly, just that it crashes when I call the function, doesn't execute a single line from it but executes everything before it.

codewalker (394)
It crashes at/in which function? if the entire code is not too big post the code here. If the code is big then post the code segment where you are making the function call, at least include the declaration/definitions of all parameters that you are passing to the function, in the code segment that you post.

This might help us help you better
Tiger58 (117)
The code is more than 22k lines long. That is why I didn't want to post it, it is better if someone would personally contact me if that was the case. Including the function alone would also be troublesome, since it uses other parts of the program.
Tiger58 (117)
This is where the program crashes, when the battle is called. However, it worked perfectly until I tried doing something at the function. I tried to equal the array of fighters to another array just to avoid changing it during the battle, since they are then sorted based on their SPEED.

It was after that, that my program started crashing. Even after I removed it, it doesn't run anymore. 

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
 
else if(randEncounter<=6)
					{
						currentMap.displayDialogue();
						gotoxy(xTextCoord,yTextCoord);
						cout<<"You have encountered a fire demon!";
						Sleep(1000);
						SetConsoleTextAttribute(GetStdHandle(STD_OUTPUT_HANDLE),7);

						for(int i=0;i<6;i++)
						{
							if(fighters[i].getName()=="Empty")
							{
								fighters[i]=fireDemon;
								playerCount++;
								break;
							}
						}

						battle(player,fighters,playerCount,playerAnimCol,skillAnim);

						//After battle, removes demon from battle group
						Player empty;

						for(int i=0;i<6;i++)
							if(fighters[i].getName()=="Fire Demon")
								fighters[i]=empty;

						SetConsoleTextAttribute(GetStdHandle(STD_OUTPUT_HANDLE),7);
						gotoxy(0,0);
						for(int i=0;i<30;i++)for(int j=0;j<80;j++)cout<<' ';
						gotoxy(0,0);
						currentMap.displayMap();

						if(churchQuest==1)
						{
							currentMap.displayDialogue();
							cout<<"You have found a Demon Signet!";
							churchQuest++;
							Sleep(3000);
							SetConsoleTextAttribute(GetStdHandle(STD_OUTPUT_HANDLE),7);
							gotoxy(0,0);
							for(int i=0;i<30;i++)for(int j=0;j<80;j++)cout<<' ';
							gotoxy(0,0);
							currentMap.displayMap();
						}
					}


Forgive the extra-long spaces, this thing doesn't do a good job at displaying it the way it is.

If anyone cares to see it, to help me resolve this issue, please PM me, I will send the program, the thing is that it has many files too. Hopefully you don't cringe too much at the newbie mistakes, though.
Last edited on
JLBorges (13770)
In your copy constructor, you are not initilizing or assigning to the pointer members; Player::mainHand etc.
Trying to access objects through these uninitialized pointers would result in undefined behaviour.

If these are owning pointers, do a deep copy. If not do a shallow copy.
http://en.wikipedia.org/wiki/Object_copy#Methods_of_copying
Tiger58 (117)
JLB - Would you care to check your inbox?
JLBorges (13770)
In your copy constructor Player::Player(const Player& other), make sure that every member is copied, nothing is left uninitialized.

For the pointer members, depending on the semantics, either copy the value of the pointer, or create a fresh copy of the object pointed to, and initialize the pointer to point to the copy.

For the array members, write a loop to copy every element in the array.

(What you have to do in the assignment operator would be similar).
Topic archived. No new replies allowed.