Pong-like game in C++ and Allegro5

Question

I've been working on this pong game for the last 10 days, and I'd like to know of your opinions about it, what could I change to make my code better?

 #include <stdio.h>
#include <allegro5/allegro.h>
#include "allegro5/allegro.h"
#include "allegro5/allegro_image.h"
#include <allegro5/allegro_native_dialog.h>
#include <allegro5/allegro_primitives.h>
#include <allegro5/color.h>
#include <allegro5/allegro_color.h>
#include <allegro5/allegro5.h>
#include <allegro5/color.h>
#include <allegro5/allegro_font.h>
#include <allegro5/allegro_ttf.h>
#include <iostream>
int p1score = 0;
int p2score=0;

ALLEGRO_EVENT ev;
int main(){
srand (time(NULL));
int d = 200;
int f = 200;
int x = 700;
int y = 100;
int a = 100;
int b = 100;
int ballx = 200;
int bally = 200;
int ballXSpeed = 10;
int ballYSpeed = 5;
bool ballmoving = true;
al_init_image_addon();
ALLEGRO_BITMAP *Player1;
ALLEGRO_BITMAP *Player2;
ALLEGRO_BITMAP *ball;
ALLEGRO_DISPLAY *display = NULL;

ALLEGRO_EVENT_QUEUE *eventqueue = NULL;
ALLEGRO_TIMER *timer = NULL;

bool playing = true;
al_init();
al_init_primitives_addon();
display = al_create_display(800, 600);
al_install_keyboard();
eventqueue = al_create_event_queue();
timer = al_create_timer(1.0 / 60.0);
al_register_event_source(eventqueue, al_get_keyboard_event_source());
al_register_event_source(eventqueue, al_get_display_event_source(display));
al_register_event_source(eventqueue, al_get_timer_event_source(timer));
Player1 = al_load_bitmap("brick.png");
Player2 = al_load_bitmap("brick.png");
int P1W = al_get_bitmap_width(Player1);
int P1H = al_get_bitmap_height(Player1);
int P2W = al_get_bitmap_width(Player2);
int P2H = al_get_bitmap_height(Player2);

ball = al_load_bitmap("ball.png");
int ballw = al_get_bitmap_width(ball);
int ballh = al_get_bitmap_height(ball);
while (playing){
    ballYSpeed = rand() % 7;

    al_clear_to_color(al_map_rgb(255, 255, 0));
    al_wait_for_event(eventqueue, &ev);
    al_start_timer(timer);
    if (ev.type == ALLEGRO_EVENT_TIMER){

        ballx = ballx + ballXSpeed;
        bally = bally + ballYSpeed;
        if (ballx < x+P1W && ballx+ballw>x &&bally<y+P1H && ballx+ballh>y){
            ballXSpeed = ballXSpeed * -1;
            ballYSpeed = ballYSpeed*-1;
        }
        if (ballx < a + P2W && ballx+ballw>a && bally <b + P2H &&bally+ballh>b){
            ballXSpeed = ballXSpeed*-1;
            ballYSpeed = ballYSpeed*-1;
        }
        if (ballx > 800 || bally > 600){
            ballx = 400;
            bally = 400;
            p2score++;
            std::cout << "P2 :  " << p2score << std::endl;
        }
        if (ballx<-1 || bally<-1){
            ballx = 400;
            bally = 400;
            p1score++;
            std::cout << "P1:   " << p1score << std::endl;
        }
    }
        if (ev.type = ALLEGRO_EVENT_KEY_DOWN){
            switch (ev.keyboard.keycode){
            case ALLEGRO_KEY_UP:
                y -= 20;
                break;
            case ALLEGRO_KEY_DOWN:
                y += 20;
                break;
            case ALLEGRO_KEY_W:
                b -= 20;
                break;
            case ALLEGRO_KEY_S:
                b += 20;
                break;


            }
        }

        al_draw_bitmap(Player1, x, y, 0);
        al_draw_bitmap(Player2, a, b, 0);
        al_draw_bitmap(ball, ballx, bally, 0);
        al_flip_display();
    }

al_flip_display();
al_rest(5);
}       

Answer 1

I see a number of things that could help you improve your program.

Fix the formatting

Perhaps the indentation problem is partly a cut-and-paste problem when you put it into the question, but the resulting formatting is not good, with very little indenting of code.

Fix the bugs

In this line

if (ev.type = ALLEGRO_EVENT_KEY_DOWN)

It's likely that you don't actually mean to assign the value but to test it, so it should be written instead like this:

if (ev.type == ALLEGRO_EVENT_KEY_DOWN)

Eliminate unused variables

Unused variables are a sign of poor code quality, so eliminating them should be a priority. In this code, d, f and ballmmoving are all unused according to my compiler also tells me that. Your compiler is probably also smart enough to tell you that, if you ask it to do so.

Eliminate global variables

My rewrite of this code uses no global variables, so clearly they are neither faster nor necessary. Eliminating them allows your code to be more readable and maintainable, both of which are important characteristics of well-written code. Global variables introduce messy linkages that are difficult to spot and error prone.

Check return values for errors

The calls to al_load_bitmap, al_create_event_queue, etc. can fail. You should check the return values to make sure they did not.

Eliminate "magic numbers"

This code is littered with "magic numbers," that is, unnamed constants such as 400, 7, 600, etc. Generally it's better to avoid that and give such constants meaningful names. That way, if anything ever needs to be changed, you won't have to go hunting through the code for all instances of "800" and then trying to determine if this particular 800 means the width of the window or some other constant that happens to have the same value.

Break up the code into smaller functions

Rather than having everything in one long function, it would be easier to read and maintain if each discrete step were its own function.

Think of the user

Right now there is no graceful way to end the game. This could easily be addressed by adding these three lines to the switch statment:

case ALLEGRO_KEY_Q:
    playing = false;
    break;

Additionally, the ball and the players can go off the top or bottom of the screen, making playing the game a lot more difficult and less enjoyable.

Also, I'm not an Allegro expert, but the game does not seem very responsive even though my machine is fast and has lots of memory. Something is awry.

Use objects

Each of the players, the field and the ball could all be objects which could simplify some things here. Consider transforming the code into a more object-oriented design.