Would just like some general feedback
agonvs

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}

Dizzy Egg

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.

StevenVI

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}

agonvs

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

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

Thread #615217. Printed from Allegro.cc