|
[C++] Good coding practices? |
LennyLen
Member #5,313
December 2004
|
before I convert my Malefiz project from C to C++, I thought I should brush up my C++/OOP skills with a smaller project first, so I've converted a 1P tic-tac-toe game I wrote in C yesterday to C++. Could someone check through it to see if there's anything that stands out as being bad coding? It works, which is good, and I get no warnings, but I'd like to make sure I eliminate any bad C habits I have before I embark on something bigger. I've attached the code, plus a Windows binary, but I'll paste the code here as well: main.cpp: 1#include <iostream>
2#include <allegro.h>
3#include "game.h"
4#include "player.h"
5#include "defines.h"
6
7
8using namespace std;
9
10
11int main() {
12
13 int depth; // colour depth
14
15 allegro_init();
16 install_timer();
17 install_keyboard();
18 install_mouse();
19
20 if ((depth = desktop_color_depth()) != 0)
21 set_color_depth(depth);
22 else {
23
24 cout << "Could not determine desktop colour depth." << endl;
25 exit(1);
26
27 }
28
29 if(set_gfx_mode(GFX_AUTODETECT_WINDOWED, WIDTH, HEIGHT, WIDTH, HEIGHT) != 0) {
30
31 allegro_message("Failed to set graphics mode\n");
32 exit(1);
33
34 }
35
36 game tictactoe;
37
38 tictactoe.init();
39 tictactoe.begin();
40
41 return 0;
42}
43END_OF_MAIN()
game.h: 1#ifndef GAME_H
2#define GAME_H
3
4#include <allegro.h>
5#include "player.h"
6#include "types.h"
7
8
9using namespace std;
10
11
12class game {
13
14 player_type board[3][3];
15 player_type current;
16 player_type winner;
17 player *noughts;
18 player *crosses;
19 BITMAP *buffer;
20 BITMAP *back;
21 FONT *msg_font;
22 int quit;
23
24 player_type play();
25 void reset_board();
26 void draw_board();
27 void text(int x, int y, char* msg);
28 player_type swap_player(player_type player);
29 int check_win();
30 position get_mouse_pos();
31
32 public:
33
34 void begin();
35 void init();
36
37};
38
39
40#endif
game.cpp: 1#include "game.h"
2#include "defines.h"
3
4
5using namespace std;
6
7
8void game::init() {
9
10 quit = 0;
11 reset_board();
12
13 noughts = new player(NOUGHT, "nought.bmp");
14 crosses = new player(CROSS, "cross.bmp");
15
16 buffer = create_bitmap(WIDTH, HEIGHT);
17 if (!buffer) {
18
19 allegro_message("Could not create buffer.\n");
20 exit(1);
21
22 }
23
24 back = create_bitmap(WIDTH, HEIGHT);
25 if (!back) {
26
27 allegro_message("Could not create buffer.\n");
28 exit(1);
29
30 }
31
32 msg_font = load_font("font.pcx", NULL, NULL);
33 if(!msg_font) {
34
35 allegro_message("Error loading \"font.pcx\".\n");
36 exit(1);
37 }
38
39 clear_to_color(back, WHITE); // clear the background to white
40
41 // draw the grid
42 rectfill(back, EDGE + SQUARE - 1, EDGE, EDGE + SQUARE + 1, EDGE + 3 * SQUARE, BLACK);
43 rectfill(back, EDGE + 2 * SQUARE - 1, EDGE, EDGE + 2 * SQUARE + 1, EDGE + 3 * SQUARE, BLACK);
44 rectfill(back, EDGE, EDGE + SQUARE - 1, EDGE + 3 * SQUARE, EDGE + SQUARE + 1, BLACK);
45 rectfill(back, EDGE, EDGE + 2 * SQUARE - 1, EDGE + 3 * SQUARE, EDGE + 2 * SQUARE + 1, BLACK);
46
47 show_mouse(screen);
48
49 current = NOUGHT;
50
51}
52
53void game::begin() {
54
55 while (!quit) {
56
57 reset_board(); // set all positions to NONE
58 draw_board(); // draw the inital board state
59
60 winner = play(); // play until there is a winner
61
62 draw_board(); // clears all messages drawn during game
63 if (winner == NOUGHT)
64 text(11, 175, "Player O wins.");
65 else if (winner == CROSS)
66 text(11, 175, "Player X wins.");
67 else
68 text(11, 175, "Nobody wins.");
69 text(11, 190, "Play again? (Y/N)");
70
71 for(;;) {
72
73 if(key[KEY_ESC] || key[KEY_N]) {
74
75 quit = 1; // signal to exit program
76 break;
77
78 }
79
80 if(key[KEY_Y]) break; // return to main loop
81
82 rest(1); // reduce CPU usage
83
84 }
85
86 }
87
88
89}
90
91player_type game::play() {
92
93 position mouse_pos; // the mouse position in board coordinates rather than screen coordinates
94 int turn_over = 0; // variable for determining when current turn is over
95 int game_over = 0; // variable for determining when current game is over
96 int mouse_released = 0; // needed to stop multiple clicks from registering if mouse button held down
97 int pieces = 0; // number of pieces placed on the board
98
99 do {
100
101 turn_over = 0; // turn begins
102
103 if (current == NOUGHT)
104 text(11, 175, "Player O's turn.");
105 else
106 text(11, 175, "Player X's turn.");
107
108 if (key[KEY_ESC]) { // player signals to exit program
109
110 game_over = 1;
111 quit = 1;
112
113 }
114
115 if (!(mouse_b & 1)) // mouse button not held down
116 mouse_released = 1;
117
118 if (mouse_b & 1 && mouse_released) {
119
120 mouse_pos = get_mouse_pos(); // get mouse position in board coordinates
121
122 if (mouse_pos.x >= 0 && mouse_pos.x <= 2 && mouse_pos.y >= 0 && mouse_pos.y <= 2) { // only do if mouse over playable area
123
124 if (board[mouse_pos.y][mouse_pos.x] == NONE) { // check that square is currently empty
125
126 board[mouse_pos.y][mouse_pos.x] = current; // player's piece is placed
127 turn_over = 1; // turn is now over
128 pieces++; // increment the number of pieces on the board
129
130 }
131
132 }
133
134 mouse_released = 0; // mouse has been held down
135
136 }
137
138 if(check_win()) {
139 draw_board();
140 return current; // if the game has been won, return the current player
141 }
142
143 if (pieces == 9) return NONE; // if all squares are filled, return NONE
144
145 if(turn_over) {
146
147 current = swap_player(current); // switch players
148 draw_board(); // draw the new board layout
149
150 }
151
152 rest(1); // reduce CPU usage
153
154 } while (!game_over);
155
156 return NONE; // return NONE if player has quit game
157
158}
159
160
161void game::draw_board() {
162
163 int x, y;
164
165 blit(back, buffer, 0, 0, 0, 0, back->w, back->h);
166
167 for (y = 0; y < 3; y++) {
168
169 for (x = 0; x < 3; x++) {
170
171 if (board[y][x] == NOUGHT)
172 noughts->draw(buffer, EDGE + SQUARE * x, EDGE + SQUARE * y);
173 else if (board[y][x] == CROSS)
174 crosses->draw(buffer, EDGE + SQUARE * x, EDGE + SQUARE * y);
175
176 }
177
178 }
179
180 scare_mouse(); // hide mouse while drawing to screen
181 blit(buffer, screen, 0, 0, 0, 0, buffer->w, buffer->h);
182 unscare_mouse(); // show mouse again
183
184}
185
186
187void game::reset_board() {
188
189 int x, y;
190
191 for (y = 0; y < 3; y++) {
192
193 for (x = 0; x < 3; x++)
194 board[y][x] = NONE;
195
196 }
197
198}
199
200
201void game::text(int x, int y, char* msg) {
202
203 scare_mouse(); // hide mouse while drawing to screen
204 textprintf_ex(screen, msg_font, x, y, BLACK, -1, msg);
205 unscare_mouse(); // show mouse again
206
207}
208
209
210player_type game::swap_player(player_type player) {
211
212 player_type new_player;
213
214 if (player == NOUGHT)
215 new_player = CROSS;
216 else
217 new_player = NOUGHT;
218
219 return new_player;
220
221}
222
223
224int game::check_win() {
225
226 int n;
227
228 for (n = 0; n < 3; n++) {
229
230 if (board[n][0] != NONE && board[n][0] == board[n][1] && board[n][1] == board[n][2]) return 1; // all 3 squares in a row are the same
231 if (board[0][n] != NONE && board[0][n] == board[1][n] && board[1][n] == board[2][n]) return 1; // all 3 squares ina column are the same
232
233 }
234 if (board[0][0] != NONE && board[0][0] == board[1][1] && board[1][1] == board[2][2]) return 1; // all 3 squares diagonally from top left are the same
235 if (board[0][2] != NONE && board[0][2] == board[1][1] && board[1][1] == board[2][0]) return 1; // all 3 squares diagonally from bottom left are the same
236
237 return 0; // no winner yet
238
239}
240
241
242position game::get_mouse_pos() {
243
244 position temp;
245
246 temp.x = (mouse_x - EDGE) / SQUARE;
247 temp.y = (mouse_y - EDGE) / SQUARE;
248
249 return temp;
250
251}
player.h: 1#ifndef PLAYER_H
2#define PLAYER_H
3
4#include <allegro.h>
5#include "types.h"
6
7
8using namespace std;
9
10
11class player {
12
13 player_type type;
14 BITMAP *bmp;
15
16 public:
17
18 player(player_type pt, char *filename);
19 ~player();
20 void draw(BITMAP *dest, int x, int y);
21
22};
23
24
25#endif
player.cpp: 1#include "player.h"
2
3
4using namespace std;
5
6
7player::player(player_type pt, char *filename) {
8
9 type = pt;
10 bmp = load_bitmap(filename, NULL);
11 if(!bmp) {
12
13 allegro_message("Error loading \"%s\".\n", filename);
14 exit(1);
15
16 }
17
18}
19
20
21player::~player() {
22
23 destroy_bitmap(bmp);
24
25}
26
27
28void player::draw(BITMAP *dest, int x, int y) {
29
30 draw_sprite(dest, bmp, x, y);
31
32}
types.h: 1#ifndef TYPES_H
2#define TYPES_H
3
4
5using namespace std;
6
7
8typedef enum { NONE, NOUGHT, CROSS} player_type; // enumerated constants for the player type
9
10
11struct position {
12
13 int x;
14 int y;
15
16};
17
18#endif
defines.h: #ifndef DEFINES_H #define DEFINES_H using namespace std; #define WIDTH 172 // the width of the window #define HEIGHT 232 // the height of the window #define EDGE 11 // the size of the gap between the window edge and teh board #define SQUARE 50 // the size of each square on the board #define WHITE makecol(255, 255, 255) #define BLACK makecol(0, 0, 0) #endif edit: fixed some of the code formatting.
|
brunooo
Member #8,346
February 2007
|
I have some suggestions, it is just mho. I never use "using namespace std", always use the prefix std::, because if some day you want to create some namespaces you wont get lost if there are things with the same name as std::, I took this from a book Another thing is that I use class names starting with uppercase letters. So player becomes Player, and I could use Player player In class I put the "private:" and "public:", it makes your class header looks better Things you can initialize and dont need to worry if it fails or not, I usually put on the class constructor, like initialing variables. Try to avoid doing tests and using exit at constructors or destructors. I could be wrong on some advices, I am still learning C++ |
LennyLen
Member #5,313
December 2004
|
brunooo said: I never use "using namespace std", always use the prefix std::, because if some day you want to create some namespaces you wont get lost if there are things with the same name as std::, I took this from a book I haven't really looked into namespaces yet. They didn't exist when I learnt C++. Quote: In class I put the "private:" and "public:", it makes your class header looks better No offense, but it's actually a pet hate of mine when people do that. By default, all members of a class are private, so adding the private keyword is superfluous. Quote: Things you can initialize and dont need to worry if it fails or not, I usually put on the class constructor, like initialing variables. I actually changed the game::init function to game::game just after posting that.
|
Thomas Fjellstrom
Member #476
June 2000
|
LennyLen said: No offense, but it's actually a pet hate of mine when people do that. By default, all members of a class are private, so adding the private keyword is superfluous. I do it because I prefer to put all public stuff first. class FooBarFrob { public: FooBarFrob(); void doSomething(); private: int someVar; };
-- |
kazzmir
Member #1,786
December 2001
|
brunooo said: I never use "using namespace std", always use the prefix std::, because if some day you want to create some namespaces you wont get lost if there are things with the same name as std::, I took this from a book There is some truth to that but the chances that you will write a class that conflict with the std:: namespace is quite low. Its usually more convenient to do 'using namespace std', but its your call. I wouldn't use #define anymore. Use const int instead. The reason is just so you can have better type checking (and macros are evil). |
Thomas Fjellstrom
Member #476
June 2000
|
kazzmir said: I wouldn't use #define anymore. Use const int instead. The reason is just so you can have better type checking. Or enums. -- |
Kris Asick
Member #1,424
July 2001
|
Yeah, that looks OK. As Brunooo suggested, you may wish to tag C++ library functions with std:: instead of defining "using namespace std" at the start in case you want to use other namespaces. When I create objects, I add T_ to the name of the object class and capitalize any first letters. So if I wanted to make a gamestate object class, I would call it T_GameState. 'T' stands for "Type" and is sort of a hold-over from my days programming in BASIC, not to mention a friend of mine who helped me to learn Pascal and C++ also did this. It's almost always good to create constructors for your classes to initialize all the variables they hold. But, as for destructors, unless you do something which allocates memory from within a class object, they're unnecessary. If you put any Allegro functions within the destructor though, make sure you don't make any global declarations of that class object, nor create them automatically within the scope of main(). If you do this, and quit the program, the destructors will be called and Allegro functions may end up being called AFTER Allegro has been removed from memory! Other things you will need to adapt to are the C++ try/throw/catch method of error handling and the memory allocation keywords. You can use C error handling in C++ up until you need to error check C++ functions, at which point you'll need to use a "try" block to trap errors, a "catch" block to handle them, and "throw" keywords to continue passing error handling to additional catch blocks. You'll also be seeing a lot of the "new", "delete", and "delete []" keywords for allocating memory much more easily than malloc() ever was. That's my advice. Take what you will, leave what you won't, and regardless of anything, just code the way that works for you. --- Kris Asick (Gemini) |
LennyLen
Member #5,313
December 2004
|
Thomas Fjellstrom said: I do it because I prefer to put all public stuff first. So why don't you use struct instead of class? kazzmir said: I wouldn't use #define anymore. Use const int instead. That seemed kinda 'global variablish,' though I guess a define that everyone accesses is similar. Quote: and macros are evil How so? Thomas Fjellstrom said: Or enums. Like this: typedef enum { WIDTH = 200, HEIGHT = 400} window_size;? edit: Also, I get sick of typing makecol(255, 255, 255) every time I want white, so I use #define WHITE makecol(255, 255, 255) how ould you avoid that?
|
Thomas Fjellstrom
Member #476
June 2000
|
LennyLen said: So why don't you use struct instead of class? Because as a long time C coder, that would just confuse me IMO a struct is for simple aggregate types, and a class is for complex types. Quote: Like this: typedef enum { WIDTH = 200, HEIGHT = 400} window_size;? for that enum I'd skip the name, and make it an anonymous enum, but I'm not sure how many compilers support that (mind you I don't care, I only use GCC). Quote: Also, I get sick of typing makecol(255, 255, 255) every time I want white, so I use #define WHITE makecol(255, 255, 255) how ould you avoid that? Yeah, you really can't do that with const ints afaik, as I think they need to be initialized when they are declared.. But if not, just init them after set_gfx_mode (setting the mode will generally change the format that makecol generates). -- |
Kibiz0r
Member #6,203
September 2005
|
Putting aside the pointless style issues, you have a few things that concern me. First off, though, your class design is pretty good. Your types adhere to a standard and your names are coherent and all that good stuff. So good job there. But I think using player_type for everything is a little misleading. I would expect player_type to be a member of the player class, not something you have a 3x3 grid of in your game class. Also, I think you could stand to break up your responsibilities a little more. The game class should be concerned with the high-level flow, and delegate to more specific classes. For example, buffer should be stored in a class like renderer or view. I also noticed you're using an int in a lot of places where they only hold a 1 or a 0. We have a data type for that now: it's called bool. You also have a worrying amount of inline arithmetic using numeric literals. You should replace those with constants, because a) then you know what 5 actually symbolizes, and b) if you have to change it, it's just in one spot. You also have a worrying amount of (really long!) ifs. Some even have comments describing what it does, which is a clear sign that it needs to be refactored, probably into a function with a name similar to the comment itself. There's also a fair amount of code that looks like it was copy-and-pasted. This is a usual sign that you need to make that into its own function. Whatever bits change from copy to copy become the parameters. Also, when you're doing a critical action like placing a piece, you should definitely break that into its own function, because it communicates that this action is one the fundamental actions the class takes. (Make sure the function is parametrized, too!) --- |
Mr. Accident
Member #3,508
May 2003
|
kazzmir said: There is some truth to that but the chances that you will write a class that conflict with the std:: namespace is quite low. Allegro 5 does this in some cases. The following program won't compile in MSVC9: #include <allegro5/allegro5.h> #include <iostream> using namespace std; int main() { bool test; return 0; } END_OF_MAIN() Allegro 5 redefines "bool" to "_Bool", which conflicts with std::_Bool, which is apparently just part of MSVC's standard library implementation, since the above code compiles fine on G++. The total namespace dump can indeed cause problems, and so should be used with caution if at all.
|
Kris Asick
Member #1,424
July 2001
|
At least under Windows using MSVC6, the bool object type still takes up 32-bits, just like an int. There's almost no reason to use bools unless you want to force the use of true and false keywords. If the Visual C++ 6.0 optimizer is able to group bools together I've never seen it mentioned anywhere in the MSDN libraries. Not to mention, many times when I make a variable that initially only has two states, I find later I want that variable to have more states. For example, in my current project, I have a "paused" variable. For weeks now, it only changed between 1 and 0. However, now that I've got task switching working, which automatically pauses, I had to make a way of detecting stuck keys as a result of a keyboard-shortcut task switch. I found the best way to handle this was to allow "paused" to equal 2, and to run a scan when it does and set itself back to 1 if the scan detects no stuck keys. If I was using bools, I'd have to either change it to an int and all the true/false references to 1's and 0's, or make a new variable, take up another 4 bytes of memory, and add more initialization code for the variable and checking to make sure it's set properly. My opinion: Stick with ints. --- Kris Asick (Gemini) |
LennyLen
Member #5,313
December 2004
|
Quote: Because as a long time C coder, that would just confuse me IMO a struct is for simple aggregate types, and a class is for complex types. Fair enough. While I prefer C myself, I actually learnt C++ first, so I still see class/struct as being the same thing but just with different default permissions. Quote: But I think using player_type for everything is a little misleading. I would expect player_type to be a member of the player class, not something you have a 3x3 grid of in your game class. Yeah, that was a throwback to an earlier design choice. I've removed the player class. Quote: I also noticed you're using an int in a lot of places where they only hold a 1 or a 0. We have a data type for that now: it's called bool. Oh yeah! I meant to look for those and change them. Thanks for the reminder. Quote: You also have a worrying amount of inline arithmetic using numeric literals. You should replace those with constants, because a) then you know what 5 actually symbolizes, and b) if you have to change it, it's just in one spot. Fixed. That was me being lazy. Quote: You also have a worrying amount of (really long!) ifs. Some even have comments describing what it does, which is a clear sign that it needs to be refactored, probably into a function with a name similar to the comment itself. This I do intentionally as I find long ifs less confusing than nested if statements. I know most people find the opposite to be true, but it makes it easier for me. Or did you mean the longer nested ifs? I moved one of them to its own function. Quote: There's also a fair amount of code that looks like it was copy-and-pasted. This is a usual sign that you need to make that into its own function. Whatever bits change from copy to copy become the parameters. I must have fixed that already, as I can't find any reperated code in the current state. Quote: Also, when you're doing a critical action like placing a piece, you should definitely break that into its own function, because it communicates that this action is one the fundamental actions the class takes. (Make sure the function is parametrized, too!) Good point. Done! updated code: main.cpp: 1#include <iostream>
2#include <allegro.h>
3#include "game.h"
4#include "defines.h"
5
6
7int main() {
8
9 int depth; // colour depth
10
11 allegro_init();
12 install_timer();
13 install_keyboard();
14 install_mouse();
15
16 if ((depth = desktop_color_depth()) != 0)
17 set_color_depth(depth);
18 else {
19
20 std::cout << "Could not determine desktop colour depth." << std::endl;
21 exit(1);
22 }
23
24 if(set_gfx_mode(GFX_AUTODETECT_WINDOWED, WIDTH, HEIGHT, WIDTH, HEIGHT) != 0) {
25
26 allegro_message("Failed to set graphics mode\n");
27 exit(1);
28 }
29
30 game tictactoe;
31
32 tictactoe.begin();
33
34 return 0;
35}
36END_OF_MAIN()
game.h: 1#ifndef GAME_H
2#define GAME_H
3
4#include <allegro.h>
5#include "graphic.h"
6#include "types.h"
7#include "mouse.h"
8
9
10class game {
11
12 player_type board[GRIDSIZE][GRIDSIZE];
13 player_type current;
14 player_type winner;
15 graphic *noughts;
16 graphic *crosses;
17 graphic *buffer;
18 graphic *back;
19 graphic *scr;
20 FONT *msg_font;
21 bool quit;
22
23 player_type play();
24 void reset_board();
25 void update_board(int x, int y, player_type player);
26 void draw_board();
27 void text(int x, int y, char* msg);
28 player_type swap_player(player_type player);
29 bool check_mouse();
30 bool check_win();
31
32 public:
33
34 game();
35 void begin();
36
37};
38
39
40#endif
game.cpp: 1#include "game.h"
2#include "defines.h"
3
4
5game::game() {
6
7 quit = 0;
8 reset_board();
9
10 noughts = new graphic("nought.bmp");
11 crosses = new graphic("cross.bmp");
12 buffer = new graphic(WIDTH, HEIGHT);
13 back = new graphic(WIDTH, HEIGHT);
14 scr = new graphic(screen);
15
16 msg_font = load_font("font.pcx", NULL, NULL);
17 if(!msg_font) {
18
19 allegro_message("Error loading \"font.pcx\".\n");
20 exit(1);
21 }
22
23 back->clear(WHITE); // clear the background to white
24
25 // draw the grid
26 back->rect(EDGE + SQUARE - 1, EDGE, EDGE + SQUARE + 1, EDGE + 3 * SQUARE, BLACK);
27 back->rect(EDGE + 2 * SQUARE - 1, EDGE, EDGE + 2 * SQUARE + 1, EDGE + 3 * SQUARE, BLACK);
28 back->rect(EDGE, EDGE + SQUARE - 1, EDGE + 3 * SQUARE, EDGE + SQUARE + 1, BLACK);
29 back->rect(EDGE, EDGE + 2 * SQUARE - 1, EDGE + 3 * SQUARE, EDGE + 2 * SQUARE + 1, BLACK);
30
31 show_mouse(screen);
32
33 current = NOUGHT;
34
35}
36
37
38void game::begin() {
39
40 while (!quit) {
41
42 reset_board(); // set all positions to NONE
43 draw_board(); // draw the inital board state
44
45 winner = play(); // play until there is a winner
46
47 draw_board(); // clears all messages drawn during game
48 if (winner == NOUGHT)
49 text(EDGE, TEXTLINE1, "Player O wins.");
50 else if (winner == CROSS)
51 text(EDGE, TEXTLINE1, "Player X wins.");
52 else
53 text(EDGE, TEXTLINE1, "Nobody wins.");
54 text(EDGE, TEXTLINE2, "Play again? (Y/N)");
55
56 for(;;) {
57
58 if(key[KEY_ESC] || key[KEY_N]) {
59
60 quit = 1; // signal to exit program
61 break;
62
63 }
64
65 if(key[KEY_Y]) break; // return to main loop
66
67 rest(1); // reduce CPU usage
68
69 }
70
71 }
72
73}
74
75player_type game::play() {
76
77 bool turn_over; // variable for determining when current turn is over
78 bool game_over = FALSE; // variable for determining when current game is over
79 int pieces = 0; // number of pieces placed on the board
80
81 do {
82
83 turn_over = FALSE; // turn begins
84
85 if (current == NOUGHT)
86 text(EDGE, TEXTLINE1, "Player O's turn.");
87 else
88 text(EDGE, TEXTLINE1, "Player X's turn.");
89
90 if (key[KEY_ESC]) { // player signals to exit program
91
92 game_over = TRUE;
93 quit = TRUE;
94
95 }
96
97 if (check_mouse()) {
98
99 turn_over = TRUE; // turn is now over
100 pieces++; // increment the number of pieces on the board
101
102 }
103
104 if(check_win()) {
105 draw_board();
106 return current; // if the game has been won, return the current player
107 }
108
109 if (pieces == NUMSQUARES) return NONE; // if all squares are filled, return NONE
110
111 if(turn_over) {
112
113 current = swap_player(current); // switch players
114 draw_board(); // draw the new board layout
115
116 }
117
118 rest(1); // reduce CPU usage
119
120 } while (!game_over);
121
122 return NONE; // return NONE if player has quit game
123
124}
125
126
127void game::draw_board() {
128
129 int x, y;
130
131 back->blit_to(buffer, 0, 0);
132
133 for (y = 0; y < GRIDSIZE; y++) {
134
135 for (x = 0; x < GRIDSIZE; x++) {
136
137 if (board[y][x] == NOUGHT)
138 noughts->blit_to(buffer, EDGE + SQUARE * x, EDGE + SQUARE * y);
139 else if (board[y][x] == CROSS)
140 crosses->blit_to(buffer, EDGE + SQUARE * x, EDGE + SQUARE * y);
141
142 }
143
144 }
145
146 scare_mouse(); // hide mouse while drawing to screen
147 buffer->blit_to(scr, 0, 0);
148 unscare_mouse(); // show mouse again
149
150}
151
152
153void game::reset_board() {
154
155 int x, y;
156
157 for (y = 0; y < GRIDSIZE; y++) {
158
159 for (x = 0; x < GRIDSIZE; x++)
160 board[y][x] = NONE;
161
162 }
163
164}
165
166
167void game::text(int x, int y, char* msg) {
168
169 scare_mouse(); // hide mouse while drawing to screen
170 textprintf_ex(screen, msg_font, x, y, BLACK, -1, msg);
171 unscare_mouse(); // show mouse again
172
173}
174
175
176player_type game::swap_player(player_type player) {
177
178 player_type new_player;
179
180 if (player == NOUGHT)
181 new_player = CROSS;
182 else
183 new_player = NOUGHT;
184
185 return new_player;
186
187}
188
189
190bool game::check_win() {
191
192 int n;
193
194 for (n = 0; n < GRIDSIZE; n++) {
195
196 if (board[n][0] != NONE && board[n][0] == board[n][1] && board[n][1] == board[n][2]) return TRUE; // all GRIDSIZE squares in a row are the same
197 if (board[0][n] != NONE && board[0][n] == board[1][n] && board[1][n] == board[2][n]) return TRUE; // all GRIDSIZE squares ina column are the same
198
199 }
200 if (board[0][0] != NONE && board[0][0] == board[1][1] && board[1][1] == board[2][2]) return TRUE; // all GRIDSIZE squares diagonally from top left are the same
201 if (board[0][2] != NONE && board[0][2] == board[1][1] && board[1][1] == board[2][0]) return TRUE; // all GRIDSIZE squares diagonally from bottom left are the same
202
203 return FALSE; // no winner yet
204
205}
206
207
208bool game::check_mouse() {
209
210 position mouse_pos; // the mouse position in board coordinates rather than screen coordinates
211 static bool mouse_released = FALSE; // needed to stop multiple clicks from registering if mouse button held down
212
213 if (!(mouse_b & 1)) // mouse button not held down
214 mouse_released = TRUE;
215
216 if (mouse_b & 1 && mouse_released) {
217
218 mouse_pos.get_pos(); // get mouse position in board coordinates
219
220 if (mouse_pos.x >= 0 && mouse_pos.x <= 2 && mouse_pos.y >= 0 && mouse_pos.y <= 2) { // only do if mouse over playable area
221
222 if (board[mouse_pos.y][mouse_pos.x] == NONE) { // check that square is currently empty
223
224 update_board(mouse_pos.x, mouse_pos.y, current); // player's piece is placed
225 mouse_released = FALSE; // mouse has been held down
226 return TRUE;
227
228 }
229
230 }
231
232 mouse_released = FALSE; // mouse has been held down
233
234 }
235
236 return FALSE;
237
238}
239
240
241void game::update_board(int x, int y, player_type player) {
242
243 board[y][x] = player;
244
245}
types.h: #ifndef TYPES_H #define TYPES_H typedef enum { NONE, NOUGHT, CROSS } player_type; // enumerated constants for the player type typedef enum { GRIDSIZE = 3, NUMSQUARES = 9, TEXTLINE1 = 175, TEXTLINE2 = 190, WIDTH = 172, HEIGHT = 232, EDGE = 11, SQUARE = 50 } sizes; #endif defines.h: #ifndef DEFINES_H #define DEFINES_H #define WHITE makecol(255, 255, 255) #define BLACK makecol(0, 0, 0) #endif> graphic.h: 1#ifndef PLAYER_H
2#define PLAYER_H
3
4#include <allegro.h>
5#include "types.h"
6
7
8class graphic {
9
10 BITMAP *bmp;
11
12 public:
13
14 graphic(char *filename);
15 graphic(int x, int y);
16 graphic(BITMAP *dest);
17 ~graphic();
18 void draw(BITMAP *src, int x, int y);
19 void blit_to(graphic *dest, int x, int y);
20 void clear(int color);
21 void rect(int x1, int y1, int x2, int y2, int color);
22
23};
24
25
26#endif
graphic.cpp: 1#include "graphic.h"
2
3
4graphic::graphic(char *filename) {
5
6 bmp = load_bitmap(filename, NULL);
7 if(!bmp) {
8
9 allegro_message("Error loading \"%s\".\n", filename);
10 exit(1);
11
12 }
13
14}
15
16
17graphic::graphic(int x, int y) {
18
19 bmp = create_bitmap(x, y);
20 if(!bmp) {
21
22 allegro_message("Error creating bitmap.\n");
23 exit(1);
24
25 }
26
27}
28
29
30graphic::graphic(BITMAP *dest) {
31
32 bmp = dest;
33
34}
35
36
37graphic::~graphic() {
38
39 destroy_bitmap(bmp);
40
41}
42
43
44void graphic::draw(BITMAP *src, int x, int y) {
45
46 draw_sprite(bmp, src, x, y);
47
48}
49
50
51void graphic::blit_to(graphic *dest, int x, int y) {
52
53 dest->draw(bmp, x, y);
54
55}
56
57
58void graphic::clear(int color) {
59
60 clear_to_color(bmp, color);
61
62}
63
64
65void graphic::rect(int x1, int y1, int x2, int y2, int color) {
66
67 rectfill(bmp, x1, y1, x2, y2, color);
68
69}
mouse.h: #ifndef MOUSE_H #define MOUSE_H struct position { int x; int y; void get_pos(); }; #endif mouse.cpp: I still have the two #defines for the makecol calls, but apart from that, I think I've incorporated all suggestions.
|
kazzmir
Member #1,786
December 2001
|
You should replace those exit(1) calls with throw'ing some exception. You may just call exit(1) in the catch handler anyway but its good practice and there may actually be a time when you can do something about the exception being thrown. Here is some quick code for using exceptions 1#include <exception>
2class myexception: public std::exception{
3public:
4 myexception(){}
5};
6
7void blah() throw (myexception) {
8 ...
9 throw myexception(); // note its not 'new myexception'
10}
11
12void foo(){
13 try{
14 blah();
15 } catch (const myexception & ex){
16 // do something
17 }
18}
Exceptions are nicer than sprinkled calls to exit because then your error handling is centralized. BTW, macros are evil because they introduce errors that are not readily apparent from looking at the source. Imagine something like #define foo "abcdef" int main(){ int foo = 2; ... } The error from g++ is x.c: In function ‘int main()’: x.c:3: error: expected unqualified-id before string constant If the #define was in a different header file you would be pretty confused for a while about this error. This almost exact situation happened to me when programming in windows land. Some system header file defined something like 'mode' and caused all sorts of compilation errors when I tried to use it as a variable name. |
Onewing
Member #6,152
August 2005
|
Just fyi, I try to make the public/protected/private levels in my header files make sense in an orderly manner, as any other programmer should only have to look at the header rather than the accompanying cpp file. In a project I'm working on now, a third party engine has a class that integrates another third party engine and their code kind of looks like this: 1class BaseEntity_cl : public Engine_A_BaseEntity
2{
3 // Engine_A Stuff
4 public:
5 ...
6
7 protected:
8 ...
9
10 // Engine_B Stuff
11 public:
12 ...
13
14 protected:
15 ...
16
17 // Engine_A and Engine_B conversion Stuff
18 public:
19 ...
20
21 protected:
22 ...
23};
Wish I could talk about what the engines do, because they are pretty neat, but I'm under contract. ------------ |
Schyfis
Member #9,752
May 2008
|
Thomas Fjellstrom said: Because as a long time C coder, that would just confuse me IMO a struct is for simple aggregate types, and a class is for complex types. "The only difference between structs and classes in C++ is that the members of a struct have public visibility by default, and the members of a class have private visibility by default."[1] ________________________________________________________________________________________________________ |
LennyLen
Member #5,313
December 2004
|
kazzmir said: You should replace those exit(1) calls with throw'ing some exception. Could you give an example of how to do it with the main.cpp code I have? Your example doesn't make much sense to me, and unfortunately neither do any of the online references I looked up.
|
Thomas Fjellstrom
Member #476
June 2000
|
Schyfis said: "The only difference between structs and classes in C++ is that the members of a struct have public visibility by default, and the members of a class have private visibility by default." I realize that. Doesn't change my expectations or preferences though. -- |
Schyfis
Member #9,752
May 2008
|
Oh, preference. I thought you were saying that that was the difference. ________________________________________________________________________________________________________ |
anonymous
Member #8025
November 2006
|
Mr. Accident said: Allegro 5 redefines "bool" to "_Bool" This seems like allegro's fault then. Identifiers beginning with underscore and capital letter are reserved for compiler implementators. As to structs/classes question, structs are generally used for POD types that are basically the same as C structs. I also use structs for types that have no private section at all (e.g simple function objects). LennyLen said: Could you give an example of how to do it with the main.cpp code I have? Your example doesn't make much sense to me, and unfortunately neither do any of the online references I looked up. Something like this: 1#include "graphic.h"
2#include <stdexcept>
3
4graphic::graphic(const char *filename)
5{
6 bmp = load_bitmap(filename, NULL);
7 if(!bmp) {
8 throw std::runtime_error(std::string("Error loading \"") + filename + '"');
9 }
10}
11
12int main()
13{
14 //init allegro
15 try {
16 //game stuff
17 }
18 catch (std::runtime_error& e) {
19 allegro_message(e.what());
20 //program will be exiting anyway
21 }
22 //deinit allegro
23}
P.S Your graphics class is either missing a copy constructor, or it should be declared private. Same goes for operator=. |
Speedo
Member #9,783
May 2008
|
Several of your classes are missing copy constructors and copy assignment operators. It also looks like some of your classes are questionable in the exception safety department. You're also using dynamically allocated objects a lot when there's no need for them, and fixing that will help a lot with the exception safety. kazzmir said: There is some truth to that but the chances that you will write a class that conflict with the std:: namespace is quite low. Its usually more convenient to do 'using namespace std', but its your call. It's still generally best avoided. As you move to more complex programs where you're using more than just a single library, you will almost always end up with name clashes between libraries regardless of your own code. kazzmir said: Here is some quick code for using exceptions void blah() throw (myexception) { ... throw myexception(); // note its not 'new myexception' } One point - you should basically never write exception specifications. You'll probably just want to forget that they event exist. See Herb Sutter's article on the subject |
LennyLen
Member #5,313
December 2004
|
I never created a copy constructor as no objects were ever copied. I just added constructors for the various ways that the object would be created in this program. Is one really needed?
|
Speedo
Member #9,783
May 2008
|
LennyLen said: I never created a copy constructor as no objects were ever copied. I just added constructors for the various ways that the object would be created in this program. Is one really needed? "Good practice" (you asked for it ) would be to either go ahead and write them even if you don't need them right this second (what if you come back to this code in 6 months and start writing something that copies or assigns them? boom) or explicitly declare them as private so that they can't accidentally be used. |
anonymous
Member #8025
November 2006
|
graphic *noughts; graphic *crosses; graphic *buffer; graphic *back; graphic *scr; There is no reason to allocate these dynamically. If you don't do so, your game class wouldn't leak memory, nor should you even worry about implementing the destructor (and copying)... You can also use the initializer list to call the constructors of the stack instances: As for the question on copy constructors and assignment operator for graphics, you need to decide whether instances of this should be copyable in the first place (by declaring them private and not implementing them / deriving from a simple class that does this to its copy constructor and operator=, a la boost::noncopyable), or if they should be copyable, whether each copy should get their own bitmap resource, or whether these should be shared between copies (e.g through reference counting, for which you can use shared_ptr<BITMAP>) ------ |
kazzmir
Member #1,786
December 2001
|
Speedo said: One point - you should basically never write exception specifications. You'll probably just want to forget that they event exist. See Herb Sutter's article on the subject [www.gotw.ca] Yes, C++ exception handling fails in a lot of ways and I wouldn't specify exceptions in function types except that in my latest project I was getting segfaults until I added the throw's clause. I have no idea why but I just go with the flow. (Having programmed in Java for a while where exceptions mostly work I would like to be able to specify the exceptions that can be thrown and have the compiler say something if I messed it up.) |
|
|