Would just like some general feedback http://www.allegro.cc/forums/view/615217 Allegro.cc Forum Thread matthew@allegro.cc (Matthew Leverton) Thu, 26 Mar 2015 15:56:27 +0000

Hi all,

This is just a little sample program which demonstrates scrolling and joystick functionality. I just wondered if you'd be willing to provide some feedback on my code style, cleanliness, technique etc. Keep in mind that I am just learning Allegro and this is the most progress I've made so far.

#SelectExpand
1#include<allegro5/allegro.h> 2#include<allegro5/allegro_primitives.h> 3#include<allegro5/allegro_image.h> 4#include<iostream> 5using namespace std; 6 7void render() 8{ 9 cout << endl; 10} 11 12enum directions {STILL, NORTH, NORTHEAST, EAST, SOUTHEAST, SOUTH, SOUTHWEST, WEST, NORTHWEST}; 13enum buttons {NONE,ZERO,ONE,EIGHT,NINE}; 14 15int main() 16{ 17 18 float gameTime = 0; 19 int frames = 0; 20 int gameFPS = 0; 21 if(!(al_init())) 22 cout << "Couldn't initialize Allegro." << cout; 23 if(!(al_install_joystick())) 24 cout << "Couldn't install joystick." << endl; 25 al_init_image_addon(); 26 ALLEGRO_DISPLAY *display = NULL; 27 display = al_create_display(1280,720); 28 ALLEGRO_EVENT_QUEUE *event_queue = NULL; 29 event_queue = al_create_event_queue(); 30 if(!event_queue) 31 cout << "al_create_event_queue failed." << endl; 32 ALLEGRO_TIMER *timer; 33 timer = al_create_timer(1.0/60); 34 al_register_event_source(event_queue, al_get_timer_event_source(timer)); 35 36 gameTime = al_current_time(); 37 al_register_event_source(event_queue, al_get_joystick_event_source()); 38 ALLEGRO_JOYSTICK* joy = al_get_joystick(0); 39 float ballposX=0; 40 float ballposY=0; 41 ALLEGRO_JOYSTICK_STATE jst; 42 directions dir = STILL; 43 buttons button = NONE; 44 bool render = false; 45 ALLEGRO_BITMAP *image = NULL; 46 ALLEGRO_BITMAP *texture = NULL; 47 ALLEGRO_BITMAP *parallax = NULL; 48 texture = al_load_bitmap("texture.pcx"); 49 image = al_load_bitmap("goldenball.png"); 50 parallax = al_load_bitmap("BluePaintedTiles.png"); 51 bool alive = true; 52 int shiftX=0; 53 int shiftY=0; 54 int shiftX2=0; 55 al_start_timer(timer); 56 while(alive) 57 { 58 ALLEGRO_EVENT ev; 59 al_wait_for_event(event_queue, &ev); 60 al_get_joystick_state(joy, &jst); 61 if (ev.type == ALLEGRO_EVENT_JOYSTICK_AXIS) 62 { 63 if ((jst.stick[2].axis[0] == 0) && (jst.stick[2].axis[1] == 0)) 64 dir = STILL; 65 if ((jst.stick[2].axis[0] == 0) && (jst.stick[2].axis[1] == -1)) 66 dir = NORTH; 67 if ((jst.stick[2].axis[0] == 1) && (jst.stick[2].axis[1] == -1)) 68 dir = NORTHEAST; 69 if ((jst.stick[2].axis[0] == 1) && (jst.stick[2].axis[1] == 0)) 70 dir = EAST; 71 if ((jst.stick[2].axis[0] == 1) && (jst.stick[2].axis[1] == 1)) 72 dir = SOUTHEAST; 73 if ((jst.stick[2].axis[0] == 0) && (jst.stick[2].axis[1] == 1)) 74 dir = SOUTH; 75 if ((jst.stick[2].axis[0] == -1) && (jst.stick[2].axis[1] == 1)) 76 dir = SOUTHWEST; 77 if ((jst.stick[2].axis[0] == -1) && (jst.stick[2].axis[1] == 0)) 78 dir = WEST; 79 if ((jst.stick[2].axis[0] == -1) && (jst.stick[2].axis[1] == -1)) 80 dir = NORTHWEST; 81 82 } 83 if (ev.type == ALLEGRO_EVENT_JOYSTICK_BUTTON_DOWN) 84 { 85 if (jst.button[0]) 86 button = ZERO; 87 if (jst.button[1]) 88 button = ONE; 89 if (jst.button[8]) 90 button = EIGHT; 91 if (jst.button[9]) 92 button = NINE; 93 } 94 else if (ev.type == ALLEGRO_EVENT_TIMER) 95 { 96 render = true; 97 frames++; 98 if (al_current_time() - gameTime >= 1) 99 { 100 gameTime = al_current_time(); 101 gameFPS = frames; 102 frames = 0; 103 } 104 } 105 if (render && al_is_event_queue_empty(event_queue)) 106 { 107 render = false; 108 switch (dir) 109 { 110 case NORTH: 111 ballposY -= 3; 112 if (ballposY < 0) 113 ballposY=0; 114 break; 115 case NORTHEAST: 116 ballposY -= 2; 117 ballposX += 2; 118 if (ballposY < 0) 119 ballposY = 0; 120 if (ballposX > 1152) 121 ballposX = 1152; 122 break; 123 case EAST: 124 ballposX += 3; 125 if (ballposX > 1152) 126 ballposX = 1152; 127 break; 128 case SOUTHEAST: 129 ballposY += 2; 130 ballposX += 2; 131 if (ballposY > 592) 132 ballposY = 592; 133 if (ballposX > 1152) 134 ballposX = 1152; 135 break; 136 case SOUTH: 137 ballposY += 3; 138 if (ballposY > 592) 139 ballposY = 592; 140 break; 141 case SOUTHWEST: 142 ballposX -= 2; 143 ballposY += 2; 144 if (ballposY > 592) 145 ballposY = 592; 146 if (ballposX < 0) 147 ballposX = 0; 148 break; 149 case WEST: 150 ballposX -= 3; 151 if (ballposX < 0) 152 ballposX = 0; 153 break; 154 case NORTHWEST: 155 ballposY -= 2; 156 ballposX -= 2; 157 if (ballposY < 0) 158 ballposY = 0; 159 if (ballposX < 0) 160 ballposX = 0; 161 break; 162 } 163 switch (button) 164 { 165 case ZERO: 166 if (jst.button[0]) 167 image = al_load_bitmap("goldenball.png"); 168 break; 169 case ONE: 170 if (jst.button[1]) 171 image = al_load_bitmap("silverball.png"); 172 break; 173 case EIGHT: 174 if (jst.button[8]) 175 break; 176 case NINE: 177 if (jst.button[9]) 178 alive = false; 179 break; 180 } 181 // al_draw_bitmap(image, ballposX, ballposY, 0); 182 al_flip_display(); 183 al_clear_to_color(al_map_rgb(0,0,0)); 184 for(int i = 0; i <11; i++) 185 for (int j = 0; j<7; j++) 186 al_draw_bitmap(texture, ((i*128)+shiftX),((j*128)+shiftY),0); 187 shiftX -= 8; 188 if (shiftX == -128) 189 shiftX = 0; 190 al_draw_bitmap(image, ballposX, ballposY, 0); 191 for (int i = 0; i < 11; i++) 192 { 193 al_draw_bitmap(parallax,((i*128)+shiftX2),-64,0); 194 al_draw_bitmap(parallax,((i*128)+shiftX2),656,0); 195 } 196 shiftX2 -= 16; 197 if (shiftX2 == -128) 198 shiftX2 = 0; 199 } 200 } 201 al_destroy_bitmap(image); 202 al_destroy_timer(timer); 203 al_destroy_event_queue(event_queue); 204 return 0; 205}

]]> no-reply@allegro.cc (agonvs) Wed, 25 Mar 2015 10:42:17 +0000

Looks OK, although you might want to start using variables instead of hard-coding all those numbers; that way if you want to change 128 to something else, you only have to change it once and not go through your code changing them all.

]]> no-reply@allegro.cc (Dizzy Egg) Wed, 25 Mar 2015 18:36:51 +0000

In terms of style, follow a set of rules and stick to it. It doesn't look like that's going on in here: 

// 1. Spaces after ifs are not consistent
if(!(al_init()))
if (render && al_is_event_queue_empty(event_queue))

// 2. Spaces around operators is not consistent
bool alive = true; 
int shiftX=0;

// 3. Space between function parameters is not consistent
al_draw_bitmap(texture, ((i*128)+shiftX),((j*128)+shiftY),0);

In terms of my personal preferences, two rules that I like to follow are: 

// 1. Opening braces start go on the same line
if (foo) {
  /* do stuff */
}

// 2. Single-line blocks are always enclosed in braces. This can help to prevent bugs.
if (foo) {
  doSomething();
}

// Rather than
if (foo)
  doSomething();

For technique, I agree with Dizzy Egg, get rid of all those magic numbers and define them as constants.

Structure-wise, you probably want to break up that giant main function into smaller chunks. I'd probably structure it something like this pseudocode:

#SelectExpand
1int main() 2{ 3 initializeScreen(); 4 loadData(); 5 6 while (alive) 7 { 8 ALLEGRO_EVENT ev; 9 while (al_get_next_event(event_queue, &ev)) 10 { 11 handleEvent(ev); 12 } 13 } 14 15 return 0; 16} 17 18void handleEvent(ALLEGRO_EVENT ev) 19{ 20 // Do different things based on the event type 21 // Handle input events appropriately 22 // Update positions and render every timer event 23}

]]> no-reply@allegro.cc (StevenVI) Wed, 25 Mar 2015 20:49:45 +0000

That is some good feedback, I'll definitely take it into consideration. Thank you friends! :-)

]]> no-reply@allegro.cc (agonvs) Wed, 25 Mar 2015 21:28:53 +0000 
switch (button) {
  case ZERO:
  if (jst.button[0])
    image = al_load_bitmap("goldenball.png");
  break;
  case ONE:
  if (jst.button[1])
    image = al_load_bitmap("silverball.png");
  break;

You keep loading from disk the same images, while the game is playing. It's needless disk access and you're slowly leaking memory.

Instead you should separate the resource loading: 

// done one on game startup
image_golden_ball = al_load_bitmap("goldenball.png");
image_silverball = al_load_bitmap("goldenball.png");

and the runtime image assignment: 

switch (button) {
  case ZERO:
  if (jst.button[0])
    image = image_goldenball;
  break;
  case ONE:
  if (jst.button[1])
    image = image_silverball;
  break;

]]> no-reply@allegro.cc (Audric) Thu, 26 Mar 2015 15:56:27 +0000