Infrastructure for my 2D game using Allegro

Question

I am making a small 2D game with the use of Allegro 5 library. I am making use of inheritance e.g. sprite class as the base class and player class as the child class, but I want to know if it's a good idea to approach it in this way and other ways that are better to do this, if there are any.

Main.cpp

#include "Allegro.h"

int main()
{
    Allegro *allegro = new Allegro();
    allegro->init();
    allegro->createWindow(60.0, 640, 480);
    allegro->gameLoop();

    return 0;
}

Allegro.h

#ifndef ALLEGRO_H_
#define ALLEGRO_H_

#include <allegro5/allegro.h>
#include <allegro5/allegro_image.h>

#include "Player.h"
#include "Keyboard.h"

class Allegro
{
private:
    ALLEGRO_DISPLAY *display;
    ALLEGRO_TIMER *timer;
    ALLEGRO_EVENT_QUEUE *event_queue;

    Keyboard keyboard;
    Player player;

    bool looping, redraw;

public:
    Allegro();
    ~Allegro();

    int init();
    int createWindow(float FPS, int w, int h);
    void gameLoop();
};

#endif

Allegro.cpp

#include "Allegro.h"

Allegro::Allegro()
{
    display = NULL;
    timer = NULL;
    event_queue = NULL;

    looping = true, redraw = false;
}

Allegro::~Allegro()
{
    al_destroy_event_queue(event_queue);
    al_destroy_timer(timer);
    al_destroy_display(display);
}

int Allegro::init()
{
    if (!al_init())
    {
        return -1;
    }

    return 0;
}

int Allegro::createWindow(float FPS, int width, int height)
{
    display = al_create_display(width, height);
    if (!display)
    {
        al_destroy_display(display);
        return -1;
    }

    timer = al_create_timer(1.0 / FPS);
    if (!timer)
    {
        al_destroy_timer(timer);
        al_destroy_display(display);
        return -1;
    }

    event_queue = al_create_event_queue();
    if (!event_queue)
    {
        al_destroy_event_queue(event_queue);
        al_destroy_timer(timer);
        al_destroy_display(display);
        return -1;
    }

    al_install_keyboard();
    al_init_image_addon();

    al_register_event_source(event_queue, al_get_display_event_source(display));
    al_register_event_source(event_queue, al_get_timer_event_source(timer));
    al_register_event_source(event_queue, al_get_keyboard_event_source());

    player.setBitmap("player.png");

    return 0;
}

void Allegro::gameLoop()
{
    al_start_timer(timer);
    while (looping)
    {
        ALLEGRO_EVENT ev;
        al_wait_for_event(event_queue, &ev);

        if (ev.type == ALLEGRO_EVENT_KEY_DOWN)
        {
            switch (ev.keyboard.keycode)
            {
            case ALLEGRO_KEY_UP:
                keyboard.key[UP] = true;
                break;
            case ALLEGRO_KEY_LEFT:
                keyboard.key[LEFT] = true;
                break;
            case ALLEGRO_KEY_DOWN:
                keyboard.key[DOWN] = true;
                break;
            case ALLEGRO_KEY_RIGHT:
                keyboard.key[RIGHT] = true;
                break;
            }
        }
        else if (ev.type == ALLEGRO_EVENT_KEY_UP)
        {
            switch (ev.keyboard.keycode)
            {
            case ALLEGRO_KEY_UP:
                keyboard.key[UP] = false;
                break;
            case ALLEGRO_KEY_LEFT:
                keyboard.key[LEFT] = false;
                break;
            case ALLEGRO_KEY_DOWN:
                keyboard.key[DOWN] = false;
                break;
            case ALLEGRO_KEY_RIGHT:
                keyboard.key[RIGHT] = false;
                break;
            }
        }
        if (ev.type == ALLEGRO_EVENT_TIMER)
        {
            player.doLogic(keyboard);
            redraw = true;
        }
        else if (ev.type == ALLEGRO_EVENT_DISPLAY_CLOSE)
        {
            looping = false;
        }

        if (redraw && al_is_event_queue_empty(event_queue))
        {
            redraw = false;

            al_clear_to_color(al_map_rgb(0, 0, 0));

            // Draw
            player.draw();

            al_flip_display();
        }
    }
}

Sprite.h

#ifndef SPRITE_H_
#define SPRITE_H_

#include <string>
#include <allegro5/allegro.h>
#include <allegro5/allegro_image.h>

class Sprite
{
protected:
    ALLEGRO_BITMAP *bitmap;
    int x, y;

public:
    Sprite();
    virtual ~Sprite();

    void setBitmap(std::string filePath);
    ALLEGRO_BITMAP *getBitmap();

    void draw();
};

#endif

Sprite.cpp

#include "Sprite.h"

Sprite::Sprite()
{
    bitmap = NULL;
    x = 0;
    y = 0;
}

Sprite::~Sprite()
{
}

void Sprite::setBitmap(std::string filePath)
{
    bitmap = al_load_bitmap(filePath.c_str());
}

ALLEGRO_BITMAP *Sprite::getBitmap()
{
    return bitmap;
}

void Sprite::draw()
{
    al_draw_bitmap(bitmap, x, y, NULL);
}

Player.h

#ifndef PLAYER_H_
#define PLAYER_H_

#include "Sprite.h"
#include "Keyboard.h"

class Player :
    public Sprite
{
private:
    int health, moveSpeed;

public:
    Player();
    ~Player();

    void doLogic(Keyboard keyboard);
};

#endif

Player.cpp

#include "Player.h"

Player::Player()
{
    health = 0;
    moveSpeed = 5;
}

Player::~Player()
{
}

void Player::doLogic(Keyboard keyboard)
{
    if (keyboard.key[UP])
        y -= moveSpeed;
    else if (keyboard.key[DOWN])
        y += moveSpeed;
    if (keyboard.key[LEFT])
        x -= moveSpeed;
    else if (keyboard.key[RIGHT])
        x += moveSpeed;
}

Keyboard.h

#ifndef KEYBOARD_H_
#define KEYBOARD_H_

enum keys{ UP, LEFT, DOWN, RIGHT };

class Keyboard
{
public:
    Keyboard();
    ~Keyboard();

    bool key[4];
};

#endif

Keyboard.cpp

#include "Keyboard.h"

Keyboard::Keyboard()
{
    for (int i = 0; i < 4; i++)
    {
        key[i] = false;
    }
}

Keyboard::~Keyboard()
{
}

Answer 1

Style

I find that you have a consistent and by many acceptable style. This is muy bueno. Some people (myself included) prefer some kind of prefix on member variables (for example m_) to make them stand out in the absence of fancy syntax highlighting.

A personal preference of mine is to always have public first, followed by protected and then private. For the reason that you should write code for those who are going to use it. And those who are going to use it are interested in the public API, not the implementation details. So that's the first thing they should see when they look at the header.

Other than that, there isn't really anything to complain over.

Naming

I find the class name Allegro to be a bit on the bad side as it doesn't as much refer to what the class does, but rather what technology it uses. I would much prefer Application.

Include Guards

Although not in the official standard, many (if not most) compilers support the #pragma once extension instead of the classical include guard you are using. There are some benefits to using #pragma once and it might be worth using them instead.

Init methods are a code smell

The presence of init type methods is a indicator of poor design. Where possible, classes should be constructed as close as possible to their final state.

This avoid issues where you forget to call the init method, or worse forget to call the init methods in the right order. Or call it twice...

In your case the constructor of Allegro should do the equivalent of Allegro::init() and Allegro::createWindow(...) combined, it should obtain all resources necessary to use the class. And of course your destructor should do the inverse, free all acquired resources. This is also referred to as Resource Acquisition Is Initialization (RAII) and is a generally desirable pattern.

The same goes for the Sprite class, the class isn't usable without first calling setBitmap so the constructor should do the equivalent work of setBitmap. Also I do believe that you never free the bitmap (the destructor is empty).

Accessors (setters/getters) are a code smell

If you have a setX and getX method for a variable in your class, you might as well make the variable public because you have already broken encapsulation. But we know that public variables are bad, so ergo set/get methods are also bad.

Or that's how I reason and this guy agrees with me.

When you see the presence of a set or get method you have to stop and think:

Okay why is some one manipulating the internal state of this class directly? Can't that logic be moved to this class instead?

Implement the rule of three

You really should implement the Rule of three for all your classes that manage resources to avoid subtle bugs.

The Keyboard class

This class puzzles me, I find it hard to motivate its existence as a class as it only has one public member array of booleans.

I would move the logic from the game loop that updates the keyboard state into the keyboard class. The make the key array private and add methods to query whether or not the appropriate key is pressed. While you're at it you can move the al_install_keyboard(); call there too, make the class earn its keep.

Finally, although it may be overkill for your simple game it may be worth looking into the Command Pattern which I find simplifies application/game logic significantly when you start doing anything non-trivial.

Answer 2

I am making use of inheritance e.g. sprite class as the base class and player class as the child class, but I want to know if it's a good idea to approach it in this way and other ways that are better to do this, if there are any.

Yes, there is another way which has become quite a trend in software development: Component-based design.

There are several downsides of using inheritance to reuse code or group functionality. The text linked above describes then very well, but to name a couple:

• Inheritance creates strong coupling between classes. A change in the base class will affect all of its child classes.

• It's a package deal. If you inherit from a class, you get then whole set of functionality, even the things you might not need.

Using components improves in the above by:

• 1st: It physically separates the classes. Good components are also interchangeable, unlike a fixed base-class.

• 2nd: You can pick-and-choose more easily just the components needed, unlike with the base class approach that consists of a fixed package.

Are components right for you?

It depends. Separating your code into a set of components might make it somewhat more complex. If your are only making sparse use of inheritance, then components are probably overkill. But that doesn't mean that you can't try it for learning purposes, of course.

---

A few other points:

1. Already mentioned: naming one of your classes with a name from a third party library is haphazard. The library might claim that name for itself on a future update, breaking your code.

2. You don't need to define empty destructors. Only supply one if there is special cleanup to be done inside it, or if you need to make it virtual.

3. Speaking of destructors, I see that Sprite allocated a bitmap with al_load_bitmap(), but that bitmap is never deallocated. It looks like you have forgotten to free it in the destructor. That's very likely a memory leak.

4. Never use unclear integer return values for functions that return true or false. E.g.: int Allegro::init() => it should return a bool (or maybe throw an exception, if the error is unrecoverable).

5. It is a better style to initialize member data using the constructor's initializer list. E.g.:

   Sprite::Sprite()
       : bitmap(nullptr)
       , x(0)
       , y(0)
   { }
   

   For a native type, such as int or pointer, there is no effective difference between both styles, however, when dealing with objects, if you initialize it using the assignment operator, then you will be initializing it twice. First, its default constructor gets implicitly called, then you assign a value to it. By using the initializer list, you ensure the member object gets inited by the parameterized constructor and just once.

6. Use nullptr instead of NULL if you have access to C++11. It has a few advantages, the most important being that it is an actual type, instead of an implementation defined macro.

7. Pass objects by const reference where appropriate. You are passing std::strings and Keyboard by value. Remember that in C++ the default for a function parameter is always a copy. For cases where you only need to look at the object, the proper way of passing it to functions is by using a const reference. E.g.:

   void Player::doLogic(const Keyboard & keyboard)
                        ^^^^^          ^
   

   For native types, such as integers and floats, passing by value is better though, even if you only look at the value, because the processor has registers that are built to store these types, so a copy has no cost, unlike for some complex and potentially large object, like a string, where copying may involve memory allocations and expensive processing.