C++

Forum

 
 
C++ Program Freezes/ Fails to Continue Randomly

TheRealAnonymous (4)
I'm currently learning C++ and I am working on a text-based game: battleships. It all works fine except the part that is concerned with generating the PC's ships (they are 100% randomly generated). Sometimes when the time comes for that, the program freezes/ fails to continue and the program's CPU usage rises to approximately 28%. Other times it works fine. I will just post the part that is concerned with generating the ships.

Note: Each number represents a coordinate which is changed to its respective format using another function which is not posted. Eg. "1" is changed to "A1", "8" to "B1" and so on.

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
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
232
 
void randompcships(int pcship5[5], int pcship4[4], int pcship3[3], int pcship2[2])
{
	int i, j, k, c;
	i = rand() % 2;
	if (i == 0)
	{
		j = rand() % 7;
		switch (j)
		{
		case 0: pcship5[0] = rand() % 3 + 1;
		case 1: pcship5[0] = rand() % 3 + 8;
		case 2: pcship5[0] = rand() % 3 + 15;
		case 3: pcship5[0] = rand() % 3 + 22;
		case 4: pcship5[0] = rand() % 3 + 29;
		case 5: pcship5[0] = rand() % 3 + 36;
		case 6: pcship5[0] = rand() % 3 + 43;
		}
		for (j = 1; j <= 4; j++)
		{
			pcship5[j] = pcship5[0] + j;
		}
	}
	else if (i == 1)
	{
		pcship5[0] = rand() % 21 + 1;
		for (j = 1; j <= 4; j++)
		{
			pcship5[j] = pcship5[0] + j * 7;
		}
	}
	i = rand() % 2;
	if (i == 0)
	{
		do
		{
			c = 0;
			j = rand() % 7;
			switch (j)
			{
			case 0: pcship4[0] = rand() % 4 + 1;
			case 1: pcship4[0] = rand() % 4 + 8;
			case 2: pcship4[0] = rand() % 4 + 15;
			case 3: pcship4[0] = rand() % 4 + 22;
			case 4: pcship4[0] = rand() % 4 + 29;
			case 5: pcship4[0] = rand() % 4 + 36;
			case 6: pcship4[0] = rand() % 4 + 43;
			}
			for (j = 1; j <= 3; j++)
			{
				pcship4[j] = pcship4[0] + j;
			}
			for (j = 0; j <= 3; j++)
			{
				for (k = 0; k <= 4; k++)
				{
					if (pcship4[j] == pcship5[k])
						c++;
				}
			}
		} while (c != 0);
	}
	else if (i == 1)
	{
		do
		{
			c = 0;
			pcship4[0] = rand() % 28 + 1;
			for (j = 1; j <= 3; j++)
			{
				pcship4[j] = pcship4[0] + j * 7;
			}
			for (j = 0; j <= 3; j++)
			{
				for (k = 0; k <= 4; k++)
				{
					if (pcship4[j] == pcship5[k])
						c++;
				}
			}
		} while (c != 0);
	}
	i = rand() % 2;
	if (i == 0)
	{
	    do
		{
			c = 0;
			j = rand() % 7;
			switch (j)
			{
			case 0: pcship3[0] = rand() % 5 + 1;
			case 1: pcship3[0] = rand() % 5 + 8;
			case 2: pcship3[0] = rand() % 5 + 15;
			case 3: pcship3[0] = rand() % 5 + 22;
			case 4: pcship3[0] = rand() % 5 + 29;
			case 5: pcship3[0] = rand() % 5 + 36;
			case 6: pcship3[0] = rand() % 5 + 43;
			}
			for (j = 1; j <= 2; j++)
			{
				pcship3[j] = pcship3[0] + j;
			}
			for (j = 0; j <= 2; j++)
			{
				for (k = 0; k <= 4; k++)
				{
					if (pcship3[j] == pcship5[k])
						c++;
				}
			}
			for (j = 0; j <= 2; j++)
			{
				for (k = 0; k <= 3; k++)
				{
					if (pcship3[j] == pcship4[k])
						c++;
				}
			}
		} while (c != 0);
	}
	else if (i == 1)
	{
		do
		{
			c = 0;
			pcship3[0] = rand() % 35 + 1;
			for (j = 1; j <= 2; j++)
			{
				pcship3[j] = pcship3[0] + j * 7;
			}
			for (j = 0; j <= 2; j++)
			{
				for (k = 0; k <= 4; k++)
				{
					if (pcship3[j] == pcship5[k])
						c++;
				}
			}
			for (j = 0; j <= 2; j++)
			{
				for (k = 0; k <= 3; k++)
				{
					if (pcship3[j] == pcship4[k])
						c++;
				}
			}
		} while (c != 0);
	}
		i = rand() % 2;
		if (i == 0)
		{
			do
			{
				c = 0;
				j = rand() % 7;
				switch (j)
				{
				case 0: pcship2[0] = rand() % 6 + 1;
				case 1: pcship2[0] = rand() % 6 + 8;
				case 2: pcship2[0] = rand() % 6 + 15;
				case 3: pcship2[0] = rand() % 6 + 22;
				case 4: pcship2[0] = rand() % 6 + 29;
				case 5: pcship2[0] = rand() % 6 + 36;
				case 6: pcship2[0] = rand() % 6 + 43;
				}
				for (j = 1; j <= 1; j++)
				{
					pcship2[j] = pcship2[0] + j;
				}
				for (j = 0; j <= 1; j++)
				{
					for (k = 0; k <= 4; k++)
					{
						if (pcship2[j] == pcship5[k])
							c++;
					}
				}
				for (j = 0; j <= 1; j++)
				{
					for (k = 0; k <= 3; k++)
					{
						if (pcship2[j] == pcship4[k])
							c++;
					}
				}
				for (j = 0; j <= 1; j++)
				{
					for (k = 0; k <= 2; k++)
					{
						if (pcship2[j] == pcship3[k])
							c++;
					}
				}
			} while (c != 0);
	}
	else if (i == 1)
	{
		do
		{
			c = 0;
			pcship2[0] = rand() % 42 + 1;
			for (j = 1; j <= 1; j++)
			{
				pcship2[j] = pcship2[0] + 7;
			}
			for (j = 0; j <= 1; j++)
			{
				for (k = 0; k <= 4; k++)
				{
					if (pcship2[j] == pcship5[k])
						c++;
				}
			}
			for (j = 0; j <= 1; j++)
			{
				for (k = 0; k <= 3; k++)
				{
					if (pcship2[j] == pcship4[k])
						c++;
				}
			}
			for (j = 0; j <= 1; j++)
			{
				for (k = 0; k <= 2; k++)
				{
					if (pcship2[j] == pcship3[k])
						c++;
				}
			}
		} while (c != 0);
	}
}
closed account (10X9216C)
You are missing the break statements in your switchs.
1
2
3
4
5
 
switch(...)
{
case 0: ... break;
case 1: ... break;
}
TheRealAnonymous (4)
I completely forgot about that... too much coding. Thanks a lot! :)
TheIdeasMan (6781)
Hi,

I have some thoughts to improve your code a bit.

I completely forgot about that... too much coding.


Yes, IMO you are writing too much hard to understand code 8+)

After writing a bunch of comments below, I decide it might be better for you to write some pseudo-code. Start with very general instructions (as comments in your editor) for completing your program, then go back & repeatedly refine them, until you are happy to write code. Leave the comments there as they serve as documentation.

Sorry if it sounds as though I have poured icy water all over your code. I know people spend spends hours writing lots of code, then someone comes along & basically says "start again". I urge you to take on board this advice, as your learning outcome will be much better if you actually learn how to write code well.

232 LOC is too much for one function - there is a "rule" that functions should be less than 40 LOC. Also functions should do one thing, and one should use them to do a "Divide & Conquer" approach. Both these ideas will make your code easier to read, understand & debug.

Candidates for functions include compound statements (code inside braces), such as in loops and switch statement cases - although you might not need any switches in your code.

So, to me lines 10 - 16, 40 - 46, etc all look very similar, as does the code surrounding them. The first number starts at 3 and increases by 1 in each successive group of code. The second number starts at 1 and increases by 7 on each line, these numbers are same for each group of code. Btw it is not a good idea to have "magic numbers" like this in your code - consider making them variables or enums instead. Lines 4 - 30 look similar, can you figure out how to write 1 function that can be called multiple times to achieve the same thing? If you are writing the same code over & over then that means there is a better way.

With this:

1
2
3
4
5
6
7
8
9
10
11
 
j = rand() % 7;
		switch (j)
		{
		case 0: pcship5[0] = rand() % 3 + 1;
		case 1: pcship5[0] = rand() % 3 + 8;
		case 2: pcship5[0] = rand() % 3 + 15;
		case 3: pcship5[0] = rand() % 3 + 22;
		case 4: pcship5[0] = rand() % 3 + 29;
		case 5: pcship5[0] = rand() % 3 + 36;
		case 6: pcship5[0] = rand() % 3 + 43;
		}


You are repeatedly setting pcship5[0] so the result is the same as setting it once in case 6:. 

1
2
3
4
5
6
7
8
9
10
11
 
j = rand() % 7;
		switch (j)
		{
		case 0: pcship5[0] = rand() % 3 + 1;
		case 1: pcship5[0] = rand() % 3 + 8;
		case 2: pcship5[0] = rand() % 3 + 15;
		case 3: pcship5[0] = rand() % 3 + 22;
		case 4: pcship5[0] = rand() % 3 + 29;
		case 5: pcship5[0] = rand() % 3 + 36;
		case 6: pcship5[0] = rand() % 3 + 43;
		}


Did you mean pcship5[j] ?

If so, you are generating a random number which is used to set an array subscript, but it looks as though you just want to set all the values in the array - why not do that with a for loop, not a switch?

Try giving your variable more meaningful names - that will making understanding easier for everyone - including yourself. Good variable names make the code self documenting. There can be problems using i and j for variable names - they look too similar, one typo can cause all kinds of problems that are really hard to see. Use words like row or col if that is what the variable is for. I always use words or words joined together for variable names.

Also, your code is in desperate need of comments, notwithstanding what I said about variable names.

Hope this helps a lot, and I look forward to seeing your new code.

Topic archived. No new replies allowed.