Allegro.cc - Online Community

Allegro.cc Forums » Programming Questions » Code Review

This thread is locked; no one can reply to it. rss feed Print
Code Review
Charles256
Member #8,370
February 2007

Warning: If you do not want to read code for the sake of review purposes run away! only reason I ask is for some review is because I'm new to the allegro library and not a huge fan of game programming all in one's style so let me know what you think of mine and what I could possibly improve or a feature of allegro I'm misusing, whatever! Let me know. :-D

main.cpp

1#include <allegro.h>
2#include <string>
3#include <vector>
4#include "buffering.cpp"
5#include "mouse_function.cpp"
6#include "main_menu.cpp"
7 
8using namespace std;
9 
10void setup()
11{
12 allegro_init();
13 set_color_depth(16);
14 install_keyboard();
15 install_mouse();
16 install_timer(); /*don't know if you need this*/
17 if (set_gfx_mode(GFX_AUTODETECT_WINDOWED,1024,768,0,0)!=0)
18 {
19 set_gfx_mode(GFX_TEXT,0,0,0,0);
20 allegro_message(allegro_error);
21 exit(1);
22 }
23 show_mouse(screen);
24}
25 
26int main(void)
27{
28 setup(); // sets up the screen
29 buffering game_screen; // creates an object where our screen will be held, handles
30 // double buffering for us.
31 main_menu my_main_menu(game_screen); // we want to set up the main menu so we pass
32 // it a reference to game screen so it knows where to set up stuff. kind of important :-D
33 
34 while(!key[KEY_ESC])
35 {
36 my_main_menu.render_menu();// puts the main menu onto the buffer
37 game_screen.render(); // puts the buffer onto the viewable screen removing the flickering
38 }
39 game_screen.destroy_bitmaps();//destroys all the bitmaps on the screen to free up memory.
40 allegro_exit();
41 return 0;
42}
43END_OF_MAIN()

buffering.cpp

1#include <allegro.h>
2#include <string>
3#include <vector>
4 
5using namespace std;
6#ifndef BUFFERING
7#define BUFFERING
8class buffering
9{
10 private:
11 // we'l need a buffer and a vector to hold reference to location of bitmaps.
12 BITMAP * buffer;
13 vector<BITMAP *> bitmaps;
14 
15 public:
16 buffering()
17 {
18 // the buffer needs to be the entire screen
19 // so we use allegro defined variables to make it so.
20 buffer=create_bitmap(SCREEN_W,SCREEN_H);
21 }
22 
23 // this is just a wrapper for load_bitmap so we can add their memory locations
24 // into a vector for automatic deletion later on.
25 void add_bitmap(BITMAP *picture,int x=0,int y=0)
26 {
27 draw_sprite(buffer,picture,x,y);
28 bool exists=false;
29 for(int i=0;i<bitmaps.size();i++)
30 {
31 if(picture==bitmaps<i>)
32 {
33 exists=true;
34 }
35 }
36 if(!exists)
37 {
38 bitmaps.push_back(picture);
39 }
40 }
41 
42 // same thign as above
43 void add_stretched_bitmap(BITMAP * picture,int x=0, int y=0,int width=SCREEN_W, int height=SCREEN_H)
44 {
45 stretch_sprite(buffer,picture,x,y,width,height);
46 bool exists=false;
47 for(int i=0;i<bitmaps.size();i++)
48 {
49 if(picture==bitmaps<i>)
50 {
51 exists=true;
52 }
53 }
54 if(!exists)
55 {
56 bitmaps.push_back(picture);
57 }
58 }
59 
60 // draw the buffer to the screen
61 void render()
62 {
63 vsync();
64 draw_sprite(screen,buffer,0,0);
65 rest(60);
66 }
67 
68 //dynamically destroy all the bitmaps. good stuff. :-D
69 // beats the hell out of 20 million calls to destroy_bitmap
70 void destroy_bitmaps()
71 {
72 for(int i=0;i<bitmaps.size();i++)
73 {
74 destroy_bitmap(bitmaps<i>);
75 }
76 }
77};
78 
79#endif

mouse_function.cpp

1#include <allegro.h>
2#include <string>
3#include <vector>
4#ifndef MOUSE_FUNCTION
5#define MOUSE_FUNCTION
6class mouse_function
7{
8public:
9 mouse_function()
10 {
11 //something, right now this class is just to see if mouse is hovering over a position
12 // later on it will expand to do other things
13 }
14 bool mouse_hovering(int x,int y, int end_x,int end_y)
15 {
16 if(mouse_x>=x && mouse_y>=y && mouse_x<=(x+end_x) && mouse_y<=(y+end_y))
17 {
18 return true;
19 }
20 else
21 {
22 return false;
23 }
24 }
25};
26 
27#endif

main_menu.cpp

1#include <allegro.h>
2#include <string>
3#include <vector>
4#include "buffering.cpp"
5#include "mouse_function.cpp"
6#ifndef MAIN_MENU
7#define MAIN_MENU
8class main_menu
9{
10private:
11 BITMAP *title,*new_game,*new_game_hover,*load_game,*load_game_hover,*delete_game,
12 *delete_game_hover,*quit,*quit_hover;
13 buffering game_screen;
14 mouse_function my_mouse;
15public:
16 main_menu(buffering the_screen)
17 {
18 // make our game_screen point to same game_Screen as main function
19 // and then load up all our bitmaps.
20 this->game_screen=the_screen;
21 title = load_bitmap("images/title.bmp", NULL);
22 new_game = load_bitmap("images/new_game.bmp", NULL);
23 new_game_hover = load_bitmap("images/new_game_hover.bmp", NULL);
24 load_game = load_bitmap("images/load_game.bmp", NULL);
25 load_game_hover = load_bitmap("images/load_game_hover.bmp", NULL);
26 delete_game = load_bitmap("images/delete_game.bmp", NULL);
27 delete_game_hover = load_bitmap("images/delete_game_hover.bmp", NULL);
28 quit = load_bitmap("images/quit_game.bmp", NULL);
29 quit_hover = load_bitmap("images/quit_hover.bmp", NULL);
30 }
31 void render_menu()
32 {
33 // here we add all our bitmaps to the menu based on whether ro not
34 // they're being hovered over.
35 
36 game_screen.add_stretched_bitmap(title,0,0);
37 if(my_mouse.mouse_hovering(400,100,new_game->w,new_game->h))
38 {
39 game_screen.add_bitmap(new_game_hover,400,100);
40 }
41 else
42 {
43 game_screen.add_bitmap(new_game,400,100);
44 }
45 if(my_mouse.mouse_hovering(400,150,load_game->w,load_game->h))
46 {
47 game_screen.add_bitmap(load_game_hover,400,150);
48 }
49 else
50 {
51 game_screen.add_bitmap(load_game,400,150);
52 }
53 if(my_mouse.mouse_hovering(400,200,delete_game->w,delete_game->h))
54 {
55 game_screen.add_bitmap(delete_game_hover,400,200);
56 }
57 else
58 {
59 game_screen.add_bitmap(delete_game,400,200);
60 }
61 if(my_mouse.mouse_hovering(400,250,quit->w,quit->h))
62 {
63 game_screen.add_bitmap(quit_hover,400,250);
64 }
65 else
66 {
67 game_screen.add_bitmap(quit,400,250);
68 }
69 }
70};
71 
72#endif;

miran
Member #2,407
June 2002

I only skimmed over the first two code boxes.

1. You should not include .cpp files. Include .h instead.
2. You do not need to call allegro_exit.
3. I don't follow the logic in the buffering class. What's the purpose of adding bitmaps? At which point do you clear the buffer or part of it in order to redraw something that has changed? EDIT: Aaah, I get it now. Yeah, should work even if it's not how most people would do it.
4. When do you destroy the buffer? As I see it you have large memory leaks especially when you pass stuff around by value.
5. Don't use rest for timing purposes.

Others will no doubt point out more problems.

--
sig used to be here

orz
Member #565
August 2000

Your program is not fullscreen, but uses 16 bpp regardless of desktop depth. You might consider matching the desktop depth or using 32 bpp or something.

draw_sprite the buffer to the screen? draw_sprite skips mask colored pixels ie two level transparency. You probably want blit there.

You also rest for 60 milliseconds after each frame is drawn. That means even if everything else is infinitely fast, your framerate is capped at 16-17 fps. Using rest() to set the rate of game play is generally frowned upon - for most things using a timer or something is better.

You do a linear search through your set of bitmap pointers. Might as well make it a set instead.

Why is drawing them to the buffer linked to marking them for deletion? They should be marked for deletion on load, not on render.

You do not remove bitmaps from the vector when you destroy them. Which is probably fine, assumeing you only call the destroy() method at the end, but still, it's bad form.

In mouse_hovering you use x_end and y_end as the width and height of the region... to me the parameter names imply that they are absolute coordinates. Of course, STL already uses misleading names, so why shouldn't everyone.

Your indentation is inconsistent. It may appear consistent to you depending upon your editor settings, but you use 4 spaces in some places and 1 tab in other places.

You have two bufferings named game_screen. The contents of one are copied over the other. This is strange, and probably not what you want to be doing. I would suggest that you make main_menu's buffering be a reference.

Buffering apparently has some methods not shown. Perhaps they're in the header file?

Charles256
Member #8,370
February 2007

I'll reply to and change some of those things later ( bout to head to class) but I just had to ask before I run out, what do you mean buffer has more methods not shown? Pretty sure I showed them all. :-D Either way, yes. I'll reply to the rest later. Man, y'all giving some tough love. :-D I'm not new to c++ just new to allegro. I'm pretty sure I was passing stuff around by pointer. Hence why me passing game_screen still allowed game_Screen.render() in main to work because it had been passed by pointer instead of by reference because by reference would cause it not to show anything when it tried to render... Of course you could be referring to something else, I'll check back in later. By the way, when you give criticism on my allegro technique (or c++ technique, but hopefully that'll be less frequent :) ) please tell me what I should be doing instead because I'm not exactly an allegro expert: )

Edit:
changed draw_sprite to blit, function call looks like this'

    void render()
    {
      vsync();
      blit(buffer,screen,0,0,0,0,SCREEN_W,SCREEN_H);
      rest(5);
    }

I agreed with your marking them for deletion on load, mainly because it's less work the program has to do hence faster, so I changed my code for main_menu to the following

1main_menu(buffering the_screen)
2 {
3 // make our game_screen point to same game_Screen as main function
4 // and then load up all our bitmaps.
5 this->game_screen=the_screen;
6 title = load_bitmap("images/title.bmp", NULL);
7 new_game = load_bitmap("images/new_game.bmp", NULL);
8 new_game_hover = load_bitmap("images/new_game_hover.bmp", NULL);
9 load_game = load_bitmap("images/load_game.bmp", NULL);
10 load_game_hover = load_bitmap("images/load_game_hover.bmp", NULL);
11 delete_game = load_bitmap("images/delete_game.bmp", NULL);
12 delete_game_hover = load_bitmap("images/delete_game_hover.bmp", NULL);
13 quit = load_bitmap("images/quit_game.bmp", NULL);
14 quit_hover = load_bitmap("images/quit_hover.bmp", NULL);
15 game_screen.mark_bitmap_for_deletion(title);
16 game_screen.mark_bitmap_for_deletion(new_game);
17 game_screen.mark_bitmap_for_deletion(new_game_hover);
18 game_screen.mark_bitmap_for_deletion(load_game);
19 game_screen.mark_bitmap_for_deletion(load_game_hover);
20 game_screen.mark_bitmap_for_deletion(delete_game);
21 game_screen.mark_bitmap_for_deletion(delete_game_hover);
22 game_screen.mark_bitmap_for_deletion(quit);
23 game_screen.mark_bitmap_for_deletion(quit_hover);
24 }

and the function mark_bitmap_for_deletion resides in buffering and looks like so

    void mark_bitmap_for_deletion(BITMAP *picture)
    {
      bitmaps.push_back(picture);
    }

changed delete_bitmaps to a destructor, after i removed allegro_exit call so it'd still work. Only issue, I wanted to do delete bitmaps; but my compiler got pretty upset about that.

    ~buffering()
    {
      for(int i=0;i<bitmaps.size();i++)
      {
        destroy_bitmap(bitmaps<i>);
      }
      bitmaps.empty();
    }

As for the mouse hovering opinion you're absolutely right, I meant to rename that before I posted. :-[

Miran, I'll reply to you in a bit but if I don't get going I swear I'll be killed by my teacher. Thanks for the comments, keep them coming!

New code looks as follows!

main.cpp

1#include <allegro.h>
2#include <string>
3#include <vector>
4#include "buffering.h"
5#include "mouse_function.h"
6#include "main_menu.h"
7 
8using namespace std;
9 
10void setup()
11{
12 allegro_init();
13 set_color_depth(32);
14 install_keyboard();
15 install_mouse();
16 install_timer(); /*don't know if you need this*/
17 if (set_gfx_mode(GFX_AUTODETECT_WINDOWED,1024,768,0,0)!=0)
18 {
19 set_gfx_mode(GFX_TEXT,0,0,0,0);
20 allegro_message(allegro_error);
21 exit(1);
22 }
23 show_mouse(screen);
24}
25 
26int main(void)
27{
28 setup(); // sets up the screen
29 buffering game_screen; // creates an object where our screen will be held, handles
30 // double buffering for us.
31 main_menu my_main_menu(game_screen); // we want to set up the main menu so we pass
32 // it a reference to game screen so it knows where to set up stuff. kind of important :-D
33 
34 while(!key[KEY_ESC])
35 {
36 my_main_menu.render_menu();// puts the main menu onto the buffer
37 game_screen.render(); // puts the buffer onto the viewable screen removing the flickering
38 }
39 return 0;
40}
41END_OF_MAIN()

buffering.h

1#include <allegro.h>
2#include <string>
3#include <vector>
4 
5using namespace std;
6#ifndef BUFFERING
7#define BUFFERING
8class buffering
9{
10 private:
11 // we'l need a buffer and a vector to hold reference to location of bitmaps.
12 BITMAP * buffer;
13 vector<BITMAP *> bitmaps;
14 
15 public:
16 buffering()
17 {
18 // the buffer needs to be the entire screen
19 // so we use allegro defined variables to make it so.
20 buffer=create_bitmap(SCREEN_W,SCREEN_H);
21 }
22 void mark_bitmap_for_deletion(BITMAP *picture)
23 {
24 bitmaps.push_back(picture);
25 }
26 
27 // this is just a wrapper for load_bitmap so we can add their memory locations
28 // into a vector for automatic deletion later on.
29 void add_bitmap(BITMAP *picture,int x=0,int y=0)
30 {
31 draw_sprite(buffer,picture,x,y);
32 }
33 
34 // same thign as above
35 void add_stretched_bitmap(BITMAP * picture,int x=0, int y=0,int width=SCREEN_W, int height=SCREEN_H)
36 {
37 stretch_sprite(buffer,picture,x,y,width,height);
38 }
39 
40 // draw the buffer to the screen
41 void render()
42 {
43 vsync();
44 blit(buffer,screen,0,0,0,0,SCREEN_W,SCREEN_H);
45 rest(5);
46 }
47 
48 //dynamically destroy all the bitmaps. good stuff. :-D
49 // beats the hell out of 20 million calls to destroy_bitmap
50 ~buffering()
51 {
52 for(int i=0;i<bitmaps.size();i++)
53 {
54 destroy_bitmap(bitmaps<i>);
55 }
56 bitmaps.empty();
57 }
58};
59 
60#endif

mouse_function.h

1#include <allegro.h>
2#include <string>
3#include <vector>
4#ifndef MOUSE_FUNCTION
5#define MOUSE_FUNCTION
6class mouse_function
7{
8public:
9 mouse_function()
10 {
11 //something, right now this class is just to see if mouse is hovering over a position
12 // later on it will expand to do other things
13 }
14 bool mouse_hovering(int x,int y, int width,int height)
15 {
16 if(mouse_x>=x && mouse_y>=y && mouse_x<=(x+width) && mouse_y<=(y+height))
17 {
18 return true;
19 }
20 else
21 {
22 return false;
23 }
24 }
25};
26 
27#endif

main_menu.h

1#include <allegro.h>
2#include <string>
3#include <vector>
4#include "buffering.h"
5#include "mouse_function.h"
6#ifndef MAIN_MENU
7#define MAIN_MENU
8class main_menu
9{
10private:
11 BITMAP *title,*new_game,*new_game_hover,*load_game,*load_game_hover,*delete_game,
12 *delete_game_hover,*quit,*quit_hover;
13 buffering game_screen;
14 mouse_function my_mouse;
15public:
16 main_menu(buffering the_screen)
17 {
18 // make our game_screen point to same game_Screen as main function
19 // and then load up all our bitmaps.
20 this->game_screen=the_screen;
21 title = load_bitmap("images/title.bmp", NULL);
22 new_game = load_bitmap("images/new_game.bmp", NULL);
23 new_game_hover = load_bitmap("images/new_game_hover.bmp", NULL);
24 load_game = load_bitmap("images/load_game.bmp", NULL);
25 load_game_hover = load_bitmap("images/load_game_hover.bmp", NULL);
26 delete_game = load_bitmap("images/delete_game.bmp", NULL);
27 delete_game_hover = load_bitmap("images/delete_game_hover.bmp", NULL);
28 quit = load_bitmap("images/quit_game.bmp", NULL);
29 quit_hover = load_bitmap("images/quit_hover.bmp", NULL);
30 game_screen.mark_bitmap_for_deletion(title);
31 game_screen.mark_bitmap_for_deletion(new_game);
32 game_screen.mark_bitmap_for_deletion(new_game_hover);
33 game_screen.mark_bitmap_for_deletion(load_game);
34 game_screen.mark_bitmap_for_deletion(load_game_hover);
35 game_screen.mark_bitmap_for_deletion(delete_game);
36 game_screen.mark_bitmap_for_deletion(delete_game_hover);
37 game_screen.mark_bitmap_for_deletion(quit);
38 game_screen.mark_bitmap_for_deletion(quit_hover);
39 }
40 void render_menu()
41 {
42 // here we add all our bitmaps to the menu based on whether ro not
43 // they're being hovered over.
44 
45 game_screen.add_stretched_bitmap(title,0,0);
46 if(my_mouse.mouse_hovering(400,100,new_game->w,new_game->h))
47 {
48 game_screen.add_bitmap(new_game_hover,400,100);
49 }
50 else
51 {
52 game_screen.add_bitmap(new_game,400,100);
53 }
54 if(my_mouse.mouse_hovering(400,150,load_game->w,load_game->h))
55 {
56 game_screen.add_bitmap(load_game_hover,400,150);
57 }
58 else
59 {
60 game_screen.add_bitmap(load_game,400,150);
61 }
62 if(my_mouse.mouse_hovering(400,200,delete_game->w,delete_game->h))
63 {
64 game_screen.add_bitmap(delete_game_hover,400,200);
65 }
66 else
67 {
68 game_screen.add_bitmap(delete_game,400,200);
69 }
70 if(my_mouse.mouse_hovering(400,250,quit->w,quit->h))
71 {
72 game_screen.add_bitmap(quit_hover,400,250);
73 }
74 else
75 {
76 game_screen.add_bitmap(quit,400,250);
77 }
78 }
79};
80 
81#endif;

Go to: