[C++] Good coding practices?
LennyLen

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:

#SelectExpand
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:

#SelectExpand
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:

#SelectExpand
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:

#SelectExpand
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:

#SelectExpand
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:

#SelectExpand
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

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++ :-X

LennyLen
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
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
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
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

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. :P

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. ;)

LennyLen

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?

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
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

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
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. :P

Kris Asick

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. ;)

LennyLen
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:

#SelectExpand
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:

#SelectExpand
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:

#SelectExpand
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:

#SelectExpand
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:

#SelectExpand
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:

#include <allegro.h>
#include "types.h"
#include "mouse.h"


void position::get_pos() {

    x = (mouse_x - EDGE) / SQUARE;
    y = (mouse_y - EDGE) / SQUARE;

}

I still have the two #defines for the makecol calls, but apart from that, I think I've incorporated all suggestions.

kazzmir

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

#SelectExpand
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

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:

#SelectExpand
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

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
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
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

Oh, preference. I thought you were saying that that was the difference.

anonymous

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:

#SelectExpand
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

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

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
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
    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:

game::game():
    quit(0),
    noughts("nought.bmp"),
    crosses("cross.bmp"),
    buffer(WIDTH, HEIGHT),
    back(WIDTH, HEIGHT),
    scr(screen), //is it a good idea to have screen destroyed by the destructor???
    msg_font(load_font("font.pcx", NULL, NULL))
{
    //...
}

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>)

------
P.S I have a problem with a script on this thread (line numbering of code boxes)? Does it perhaps take too much time, seeing that there are many code boxes here? (Toggling line numbers on/off won't work when the script is stopped.)

kazzmir
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.)

Indeterminatus

Just a minor issue and not C++-specific:

if ((depth = desktop_color_depth()) != 0)

I'd rather see this one split up; it's overloaded, thus more difficult to read, and most of all, it invites errors involving = and ==.

To put it more bluntly and dogmatic: Never make (active) use of side-effects.

anonymous
kazzmir said:

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.

IMO it is more likely that you have something wrong elsewhere and since the error specification would lead to small changes in the binary these might cover up the symptoms (temporarily, another change might make it crash again). I've had a program segfault unless a single bool assignment was commented out (the real cause was elsewhere and the program had been working fine despite the bug for a couple of days).

BAF

I saw a bunch of points I wanted to make, but I'll only touch on two.

First off, use of enums for constants. I don't like that idea, because that's not what they're for. What's wrong with const ints again? After all... they are constants.

Second, struct vs class. Thomas is right, and if you move on to more civilized languages (like C#) it goes one step further. In C#, structs are passed by value, whereas classes are passed by reference.

Indeterminatus
BAF said:

First off, use of enums for constants. I don't like that idea, because that's not what they're for. What's wrong with const ints again? After all... they are constants.

Please note that I did not follow this thread. Nothing's wrong with const int, and nothing's wrong with enum. Both have their uses, and their domains are orthogonal. An enumeration is quite explicit in what it's there for: providing a range of values, noone should care (or have to care) how the respective values are represented "underneath".

BAF

Yeah, enums have their uses, but not for setting a constant for screen width or height, IMO.

brunooo

Asking about good practices, is it a good practice to exceed the char type limit?

For example:

char a = 127;
a += 2; // a will be -127 now right?

Instead of:

char a = 127;
if (a + 2 > 127) a = a + 2 - 256;
else a += 2;

Christopher Bludau
brunooo said:

a += 2; // a will be -127 now right?

Quote:

if (a + 2 > 127)

If your first statment would be true how would that second statement ever evaluate to true? Never, bacause -127 is not bigger then 127. You can not detect an overflow with a bigger as sign. That makes no sense.

I always thought that char can hold upto 255 values and is unsigned per default? But I could be wrong about this.

LennyLen
brunooo said:

Asking about good practices, is it a good practice to exceed the char type limit?

No it's not.

Quote:
char a = 127;
a += 2; // a will be -127 now right?

It depends on the compiler. Some treat the type char type as being signed, some do not. If char's are signed, then yes it will wrap around, if not, it will now equal 129.

If you want to be certain whether or not a char will be signed or not, use signed char or unsigned char.

brunooo

Sorry, I would than do this:

int a = 127;
signed char b;
if (a + 2 > 127) b = a + 2 - 256;
else b = a + 2;

Kibiz0r

If your first statment would be true how would that second statement ever evaluate to true? Never, bacause -127 is not bigger then 127. You can not detect an overflow with a bigger as sign. That makes no sense.

Actually, I believe numerical literals are ints by default, which means the char would get promoted to an int before the operation, so it would be 129.

Still, the whole thing is really sketchy and I would avoid the situation altogether. Overflows should be considered an error, not a tool.

bamccaig

:-X

Speedo
LennyLen said:

If you want to be certain whether or not a char will be signed or not, use signed char or unsigned char.

Well, if you're using it for text you should just use plain char, so that it's in line with whatever your compiler/project uses. If you want a 1 byte integer, you should explicitly use signed/unsigned char.

Kibiz0r said:

Actually, I believe numerical literals are ints by default

I believe they should be a signed integer type which matches the word size of the target platform (which isn't always int - wouldn't want to be too consistent or anything).

LennyLen
Speedo said:

Well, if you're using it for text you should just use plain char, so that it's in line with whatever your compiler/project uses.

Yup. char values outside of 0-127 shouldn't be used for text anyway.

OnlineCop
brunooo said:

Sorry, I would than do this:

int a = 127;
signed char b;
if (a + 2 > 127) b = a + 2 - 256;
else b = a + 2;

I would just do this:

signed char a;
// ...
if (a < 126) a += 2;
else a = 128 - a;

Would this not protect against the problem faced with the wrap-around?

LennyLen
anonymous said:

graphic *noughts;
graphic *crosses;
graphic *buffer;
graphic *back;
graphic *scr;

There is no reason to allocate these dynamically.

The reason I did that was when I did noughts("nought.bmp"), I got the following error, which I have no idea how to fix:

mingw32-g++.exe -Wall -fexceptions -O2 -c "C:\Users\LennyLen\Documents\source code\tictac++\main.cpp" -o obj\Release\main.o
mingw32-g++.exe -Wall -fexceptions -O2 -c "C:\Users\LennyLen\Documents\source code\tictac++\game.cpp" -o obj\Release\game.o
C:\Users\LennyLen\Documents\source code\tictac++\game.cpp: In constructor `game::game()':
C:\Users\LennyLen\Documents\source code\tictac++\game.cpp:5: error: no matching function for call to `graphic::graphic()'
C:\Users\LennyLen\Documents\source code\tictac++\graphic.h:17: note: candidates are: graphic::graphic(const graphic&)
C:\Users\LennyLen\Documents\source code\tictac++\graphic.h:16: note: graphic::graphic(BITMAP*)
C:\Users\LennyLen\Documents\source code\tictac++\graphic.h:15: note: graphic::graphic(int, int)
C:\Users\LennyLen\Documents\source code\tictac++\graphic.h:14: note: graphic::graphic(const char*)
C:\Users\LennyLen\Documents\source code\tictac++\game.cpp:10: error: no match for call to `(graphic) (const char[11])'

I get the same for all of those declarations if I stop them from being dynamic.

In the end, I removed the graphic class altogether. It didn't really serve any purpose.

Quote:

You can also use the initializer list to call the constructors of the stack instances:

The initializer list is something new to me. What benefit does it serve that is better than just assigning values inside the constructor?

BAF said:

Yeah, enums have their uses, but not for setting a constant for screen width or height, IMO.

It seemed a bit odd to me too, butit was suggested. I've changed them to const ints, even though it makes no real difference except stylistically.

I still need to add the exception handling, but that can wait until tomorrow.

Speedo
LennyLen said:

The reason I did that was when I did noughts("nought.bmp"), I got the following error, which I have no idea how to fix:

...

I get the same for all of those declarations if I stop them from being dynamic.

If you do not initialize them in the initializer list, they must have a default constructor that can be called. See below.

Quote:

The initializer list is something new to me. What benefit does it serve that is better than just assigning values inside the constructor?

Efficiency. Every class member must be constructed before control enters the body of the contructor. So, if you don't use the list you have a default construction plus an assignment.

It's the same logic that leads us in C++ to prefer delaying the declaration of variables as long as possible, so they can initialized on creation.

#SelectExpand
1void Foo( ) 2{ 3 string s; // default construction 4 5 // ... 20 lines later 6 7 s = "Hello World!"; // assignment 8} 9 10//better 11void Foo( ) 12{ 13 //... 20 lines later 14 15 string s("Hello World!"); // initialization 16}

anonymous
Speedo said:

Efficiency.

Not only that. There are certain things that can only be initialized, not assigned later, like classes without default constructor :), constant members, reference members and base classes.

LennyLen

I think this should be the final revison of this little project now:

main.cpp:

#SelectExpand
1#include <iostream> 2#include <stdexcept> 3#include <allegro.h> 4#include "game.h" 5#include "defines.h" 6 7 8void init_alleg(); 9 10 11int main() { 12 13 try { 14 15 init_alleg(); 16 17 game tictactoe; 18 19 tictactoe.begin(); 20 21 } 22 catch (std::runtime_error& e) { 23 24 allegro_message(e.what()); 25 return 1; 26 27 } 28 29 return 0; 30} 31END_OF_MAIN() 32 33 34void init_alleg() { 35 36 int depth; 37 38 allegro_init(); 39 install_timer(); 40 install_keyboard(); 41 install_mouse(); 42 43 if ((depth = desktop_color_depth()) != 0) 44 set_color_depth(depth); 45 else 46 throw std::runtime_error(std::string("Couldn't determine desktop resolution")); 47 48 if(set_gfx_mode(GFX_AUTODETECT_WINDOWED, WIDTH, HEIGHT, WIDTH, HEIGHT) != 0) 49 throw std::runtime_error(std::string("Failed to set graphics mode")); 50 51}

game.h:

#SelectExpand
1#ifndef GAME_H 2#define GAME_H 3 4#include <stdexcept> 5#include <allegro.h> 6#include "types.h" 7#include "defines.h" 8#include "mouse.h" 9 10 11class game { 12 13 player_type board[GRIDSIZE][GRIDSIZE]; 14 player_type current; 15 player_type winner; 16 bool quit; 17 BITMAP *noughts; 18 BITMAP *crosses; 19 BITMAP *grid; 20 BITMAP *buffer; 21 FONT *msg_font; 22 23 BITMAP *bitmap_load(const char* filename); 24 BITMAP *bitmap_create(int x, int y); 25 player_type play(); 26 void reset_board(); 27 void update_board(int x, int y, player_type player); 28 void draw_board(); 29 void text(int x, int y, char* msg); 30 player_type swap_player(player_type player); 31 bool check_mouse(); 32 bool check_win(); 33 34 public: 35 36 game(); 37 void begin(); 38 39}; 40 41 42#endif

game.cpp:

#SelectExpand
1#include "game.h" 2 3 4game::game() : quit(0), 5 noughts(bitmap_load("nought.bmp")), 6 crosses(bitmap_load("cross.bmp")), 7 grid(bitmap_load("grid.bmp")), 8 buffer(bitmap_create(WIDTH, HEIGHT)), 9 msg_font(load_font("font.pcx", NULL, NULL)) { 10 11 reset_board(); 12 13 if(!msg_font) 14 throw std::runtime_error(std::string("Error loading \"font.pcx\"")); 15 16 show_mouse(screen); 17 18 current = NOUGHT; 19 20} 21 22 23BITMAP *game::bitmap_load(const char *filename) { 24 25 BITMAP *bmp = load_bitmap(filename, NULL); 26 if(!bmp) 27 throw std::runtime_error(std::string("Error loading \"") + filename + '"'); 28 29 return bmp; 30 31} 32 33 34BITMAP *game::bitmap_create(int x, int y) { 35 36 BITMAP *bmp = create_bitmap(x, y); 37 if(!bmp) 38 throw std::runtime_error(std::string("Error creating bitmap")); 39 40 return bmp; 41 42} 43 44 45void game::begin() { 46 47 while (!quit) { 48 49 reset_board(); // set all positions to NONE 50 draw_board(); // draw the inital board state 51 52 winner = play(); // play until there is a winner 53 54 draw_board(); // clears all messages drawn during game 55 if (winner == NOUGHT) 56 text(EDGE, TEXTLINE1, "Player O wins."); 57 else if (winner == CROSS) 58 text(EDGE, TEXTLINE1, "Player X wins."); 59 else 60 text(EDGE, TEXTLINE1, "Nobody wins."); 61 text(EDGE, TEXTLINE2, "Play again? (Y/N)"); 62 63 for(;;) { 64 65 if(key[KEY_ESC] || key[KEY_N]) { 66 67 quit = 1; // signal to exit program 68 break; 69 70 } 71 72 if(key[KEY_Y]) break; // return to main loop 73 74 rest(1); // reduce CPU usage 75 76 } 77 78 } 79 80} 81 82player_type game::play() { 83 84 bool turn_over; // variable for determining when current turn is over 85 bool game_over = FALSE; // variable for determining when current game is over 86 int pieces = 0; // number of pieces placed on the board 87 88 do { 89 90 turn_over = FALSE; // turn begins 91 92 if (current == NOUGHT) 93 text(EDGE, TEXTLINE1, "Player O's turn."); 94 else 95 text(EDGE, TEXTLINE1, "Player X's turn."); 96 97 if (key[KEY_ESC]) { // player signals to exit program 98 99 game_over = TRUE; 100 quit = TRUE; 101 102 } 103 104 if (check_mouse()) { 105 106 turn_over = TRUE; // turn is now over 107 pieces++; // increment the number of pieces on the board 108 109 } 110 111 if(check_win()) { 112 draw_board(); 113 return current; // if the game has been won, return the current player 114 } 115 116 if (pieces == NUMSQUARES) return NONE; // if all squares are filled, return NONE 117 118 if(turn_over) { 119 120 current = swap_player(current); // switch players 121 draw_board(); // draw the new board layout 122 123 } 124 125 rest(1); // reduce CPU usage 126 127 } while (!game_over); 128 129 return NONE; // return NONE if player has quit game 130 131} 132 133 134void game::draw_board() { 135 136 int x, y; 137 138 blit(grid, buffer, 0, 0, 0, 0, grid->w, grid->h); 139 140 for (y = 0; y < GRIDSIZE; y++) { 141 142 for (x = 0; x < GRIDSIZE; x++) { 143 144 if (board[y][x] == NOUGHT) 145 draw_sprite(buffer, noughts, EDGE + SQUARE * x, EDGE + SQUARE * y); 146 else if (board[y][x] == CROSS) 147 draw_sprite(buffer, crosses, EDGE + SQUARE * x, EDGE + SQUARE * y); 148 149 } 150 151 } 152 153 scare_mouse(); // hide mouse while drawing to screen 154 blit(buffer, screen, 0, 0, 0, 0, buffer->w, buffer->h); 155 unscare_mouse(); // show mouse again 156 157} 158 159 160void game::reset_board() { 161 162 int x, y; 163 164 for (y = 0; y < GRIDSIZE; y++) { 165 166 for (x = 0; x < GRIDSIZE; x++) 167 board[y][x] = NONE; 168 169 } 170 171} 172 173 174void game::text(int x, int y, char* msg) { 175 176 scare_mouse(); // hide mouse while drawing to screen 177 textprintf_ex(screen, msg_font, x, y, BLACK, -1, msg); 178 unscare_mouse(); // show mouse again 179 180} 181 182 183player_type game::swap_player(player_type player) { 184 185 player_type new_player; 186 187 if (player == NOUGHT) 188 new_player = CROSS; 189 else 190 new_player = NOUGHT; 191 192 return new_player; 193 194} 195 196 197bool game::check_win() { 198 199 int n; 200 201 for (n = 0; n < GRIDSIZE; n++) { 202 203 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 204 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 205 206 } 207 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 208 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 209 210 return FALSE; // no winner yet 211 212} 213 214 215bool game::check_mouse() { 216 217 position mouse_pos; // the mouse position in board coordinates rather than screen coordinates 218 static bool mouse_released = FALSE; // needed to stop multiple clicks from registering if mouse button held down 219 220 if (!(mouse_b & 1)) // mouse button not held down 221 mouse_released = TRUE; 222 223 if (mouse_b & 1 && mouse_released) { 224 225 mouse_pos.get_pos(); // get mouse position in board coordinates 226 227 if (mouse_pos.x >= 0 && mouse_pos.x <= (GRIDSIZE - 1) && mouse_pos.y >= 0 && mouse_pos.y <= (GRIDSIZE - 1)) { // only do if mouse over playable area 228 229 if (board[mouse_pos.y][mouse_pos.x] == NONE) { // check that square is currently empty 230 231 update_board(mouse_pos.x, mouse_pos.y, current); // player's piece is placed 232 mouse_released = FALSE; // mouse has been held down 233 return TRUE; 234 235 } 236 237 } 238 239 mouse_released = FALSE; // mouse has been held down 240 241 } 242 243 return FALSE; 244 245} 246 247 248void game::update_board(int x, int y, player_type player) { 249 250 board[y][x] = player; 251 252}

mouse.h:

#SelectExpand
1#ifndef MOUSE_H 2#define MOUSE_H 3 4#include <allegro.h> 5#include "types.h" 6#include "defines.h" 7 8 9struct position { 10 11 int x; 12 int y; 13 14 void get_pos(); 15 16}; 17 18 19#endif

mouse.cpp:

#include "mouse.h"


void position::get_pos() {

    x = (mouse_x - EDGE) / SQUARE;
    y = (mouse_y - EDGE) / SQUARE;

}

types.h:

#ifndef TYPES_H
#define TYPES_H


typedef enum { NONE, NOUGHT, CROSS } player_type;  // enumerated constants for the player type


#endif

defines.h:

#ifndef DEFINES_H
#define DEFINES_H


#define WHITE makecol(255, 255, 255)
#define BLACK makecol(0, 0, 0)

const int GRIDSIZE = 3, NUMSQUARES = 9, TEXTLINE1 = 175, TEXTLINE2 = 190, WIDTH = 172, HEIGHT = 232, EDGE = 11, SQUARE = 50;


#endif

And here is all the code/resources with a Windows binary.

OnlineCop

Is there an advantage to using a #define for makecol(..., ..., ...)?

If the preprocessor simply replaces your WHITE instances with "makecol(255, 255, 255)", you are just reusing the same thing. I'd just use "const int WHITE = makecol(255, 255, 255);" and avoid any redundant function calls to makecol() if you are just using it for white (or black, or lavender, or puce, ...).

If you're worried that it's trying to set the color before setting the color depth, just don't make it a const and set its value inside your init() function.

LennyLen
OnlineCop said:

If you're worried that it's trying to set the color before setting the color depth, just don't make it a const and set its value inside your init() function.

It was more that I was trying to avoid any global variables.

OnlineCop
LennyLen said:

It was more that I was trying to avoid any global variables.

And yet Allegro 4 and 5 both use globals all over the place. Once you set the screen resolution, you have SCREEN_W, SCREEN_H, you've got mouse_b, etc. It gives you access to globals all over the place.

I think globals have their place. Sometimes you just don't want to pass some variable around through a dozen function calls to be used in one or two places down the chain: you use a global.

LennyLen
OnlineCop said:

I think globals have their place. Sometimes you just don't want to pass some variable around through a dozen function calls to be used in one or two places down the chain: you use a global.

I think #defines have their place. Sometimes you just don't want to write the same bit of text over and over in many places: you use a #define.

edit: And to be honest, the #define vs global variable has nothing to do with C++ explicitly. It's the same for any language that supports both. I'm only interested in in C++ specifics for now.

Christopher Bludau
 CResourceManager::add_color(makecol(255, 255, 255), "white");
 CResourceManager::add_color(makecol(0, 0, 0), "black");

 ..

 col = CResourceManager::get_color("white");

That's the proper way of doing it in OOP.
Not with defines. And not with globals.

Whether you make the methods static or not depends on what you need.

You could go further and define a Resource class and inherit all Resources from that... you have to decide if you want:

a) // two methods for every resource
add_a(type_a, id)
add_b(type_b, id)
type_a a = get_a(id);
type_b a = get_b(id);

or:
b) // having to cast everytime you get a resource
add(resource, id)
type_a a = (type_a)get(id);

Both have their pros and cons.

EDIT:
Obviously, if you want to take the second path, you would have to write wrappers around all allegro c types. Otherwise you can't let them extend the resource class.

Audric

col = CResourceManager::get_color("white");
I find this very error-prone. It's going to compile ok and bite you at runtime if you mistake "grey" and "gray", for example.

Christopher Bludau

load_bitmap("sprite.bmp"); might compile ok but will bite you at runtime if the image can't be loaded.

Same goes for any resource once you use a ResourceManager.

anonymous

You could also have global functions instead of macros:

int white() { return makecol(255, 255, 255); }
int black() { return makecol(0, 0, 0); }

Practically no difference with how the macro works, except takes a bit more typing, but can be put in a namespace etc. The macro itself is benign, except perhaps for naming conflicts that they all can cause.

Arthur Kalliokoski

I just use 0 and -1 for black and white :-X

anonymous

Except that is less readable, won't work on all colour depths and still doesn't solve the question what you do when you want other colours like green...

bamccaig
LennyLen said:

catch (std::runtime_error& e) {

Just for good measure, you might want to beef up the try...catch with a few more catches so no exceptions can get past your main routine. std::exception would catch any type derived from the standard exception type. And you can use an ellipsis (...) to catch anything else, and still output some kind of "WTF is this..." message so the user doesn't just have the game outright crash on him. In Intern's Quest, we catch std::invalid_argument, std::range_error, std::logic_error, std::runtime_error, and std::exception, in that order (clearly I didn't think ... was necessary at the time).

It allows for really sweet error handling in the application code. For starters, I just said everything is fatal, and would throw an exception for everything, which were only caught in main. It worked out really great because the game would typically output some kind of error message and exit gracefully for anything (Allegro complicated things because of how I was using timers, etc... Occasionally Allegro timers would not be uninstalled and would fire after Allegro was deinitialized and it would crash anyway, but I think that is mostly cleaned up now).

Speedo
bamccaig said:

Just for good measure, you might want to beef up the try...catch with a few more catches so no exceptions can get past your main routine. std::exception would catch any type derived from the standard exception type. And you can use an ellipsis (...) to catch anything else, and still output some kind of "WTF is this..." message so the user doesn't just have the game outright crash on him. In Intern's Quest, we catch std::invalid_argument, std::range_error, std::logic_error, std::runtime_error, and std::exception, in that order (clearly I didn't think ... was necessary at the time).

When you're just catching fatal errors in main, there's really not much point catching anything other than std::exception and any exceptions thrown by 3rd party libs which don't dervive from std::exception. Anything that derives from std::exception should adequately report whatever the problem was via its what() method, so there's no reason to catch derived types unless you're actually going to take some action specific to that type.

anonymous
Speedo said:

unless you're actually going to take some action specific to that type

In which case you might also derive your own exception type for a particular error (e.g bitmap_load_error). Pick one of the stdexcepts as the base, and you'll only need a constructor that passes the exception message to the base constructor. It is also good to derive them from std exceptions, so you could always catch them with std::exception& if you don't want a particular action.

Speedo
anonymous said:

In which case you might also derive your own exception type for a particular error (e.g bitmap_load_error). Pick one of the stdexcepts as the base, and you'll only need a constructor that passes the exception message to the base constructor.

It's probably better to actually make your own intermediate exception type and then derive from it, because the standard doesn't require std::exception to be able to hold a message (MSVC and some other compilers extend it to do so, GCC doesn't).

I'd also recommend taking a look at Boost.Exception. It's particularly awesome in that you can transport any arbitrary information along with any given exception.

bamccaig

Though the type of exception does carry some information as well, which I found useful to display to the user as well. At least, I found it easier to guess what went wrong when debugging, without having to write type-specific what messages.

Speedo

IMO any exception should carry enough info to tell you (the programmer) what the problem was. But even if you want to display the name of the exception class, that's done easily enough with typeid(excpetionclass).name().

bamccaig
Speedo said:

...typeid(excpetionclass)...

:o

anonymous
Speedo said:

It's probably better to actually make your own intermediate exception type and then derive from it, because the standard doesn't require std::exception to be able to hold a message (MSVC and some other compilers extend it to do so, GCC doesn't).

What I meant is use one of the derivates of the std::exception from <stdexcept> (IIRC std::exception itself doesn't even have a constructor to accept a message). Or naturally you can make your intermediary classes: std::exception > std::runtime_error > my::resource_error > my::bitmap_loading_error.

bamccaig said:

Though the type of exception does carry some information as well, which I found useful to display to the user as well. At least, I found it easier to guess what went wrong when debugging, without having to write type-specific what messages.

The type of the exception lets you catch different kinds of exceptions in different places and/or handle them separately. The what message is for a description that you fill in where the exception happens and where you have the exact information. You can also have intermediary try blocks where you catch the message, add more information to it and rethrow it (but I guess that would not be the general pattern and some things might better be left for the debugger).

Thread #600126. Printed from Allegro.cc