Allegro.cc - Online Community

Allegro.cc Forums » Programming Questions » [C++] Good coding practices?

This thread is locked; no one can reply to it. rss feed Print
 1   2   3 
[C++] Good coding practices?
Indeterminatus
Member #737
November 2000
avatar

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.

_______________________________
Indeterminatus. [Atomic Butcher]
si tacuisses, philosophus mansisses

anonymous
Member #8025
November 2006

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
Member #2,981
December 2002
avatar

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
Member #737
November 2000
avatar

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

_______________________________
Indeterminatus. [Atomic Butcher]
si tacuisses, philosophus mansisses

BAF
Member #2,981
December 2002
avatar

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

brunooo
Member #8,346
February 2007
avatar

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;

count
Member #5,401
January 2005

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
Member #5,313
December 2004
avatar

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
Member #8,346
February 2007
avatar

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
Member #6,203
September 2005
avatar

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
Member #7,536
July 2006
avatar

Speedo
Member #9,783
May 2008

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
Member #5,313
December 2004
avatar

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
Member #7,919
October 2006
avatar

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
Member #5,313
December 2004
avatar

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
Member #9,783
May 2008

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
Member #8025
November 2006

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
Member #5,313
December 2004
avatar

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
Member #7,919
October 2006
avatar

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
Member #5,313
December 2004
avatar

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
Member #7,919
October 2006
avatar

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
Member #5,313
December 2004
avatar

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.

count
Member #5,401
January 2005

 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
Member #907
January 2001

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.

count
Member #5,401
January 2005

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.

 1   2   3 


Go to: