Gradius-like game

Question

I have some experience with OOP but I'm very new to game programming. For a school assignment, here's my first attempt at making a "real" game containing Entities, a game engine, game loop and the likes. I wanted to know if I used those concepts correctly, and would love to have some tips on things I could've done better.

GitHub

Here's the Entities header, from which everything is derived (players, enemies, asteroids...):

#ifndef A_ENTITIES_HPP
# define A_ENTITIES_HPP

#include <iostream>
#include <ncurses.h>

class AEntities
{
private:
    AEntities(void);

protected:
    WINDOW const *      _win;
    int                 _x, _y;
    int                 _winwidth, _winheight;
    char                _char;
    AEntities *         _missile;

public:

    AEntities(WINDOW const * win, int x, int y, char caract);
    virtual ~AEntities(void);
    AEntities(AEntities const &src);

    AEntities &operator=(AEntities const & rhs);

    virtual void        movement() = 0;
    virtual void        update(void) = 0;

    void                draw(void);
    void                del(void);

    bool                impact(AEntities * entities) const;

    int                 getX(void) const;
    int                 getY(void) const;
    char                getChar(void) const;
    AEntities *         getMissile(void) const;

    static int          loopCount;
};

#endif

And here's the GameEngine which contains a array of pointers to Entities, for their management:

AEntities *                 _entities[ENTITIES_MAX];

Code:

#include "GameEngine.hpp"

GameEngine::GameEngine(void)
{
    initscr();
    nodelay(stdscr, TRUE);
    keypad(stdscr, TRUE);
    noecho();
    curs_set(0);
    getmaxyx(stdscr, _winheight, _winwidth);
    _p1 = new Player(stdscr, 0, _winheight / 2);
    _win = stdscr;
    for (int i = 0; i < ENTITIES_MAX; i++)
        _entities[i] = 0;
}

GameEngine::~GameEngine(void)
{
    delete _p1;
    endwin();
}

GameEngine::GameEngine(GameEngine const &src) :
_win(src._win), _winheight(src._winheight), _winwidth(src._winwidth)
{
}

GameEngine      &GameEngine::operator=(GameEngine const &src)
{
    if (this != &src)
        _win = src._win;
    return (*this);
}

bool                GameEngine::render(void)
{
    usleep(15000);
    mvprintw(0, 0, "Score: %d", _score);
    if (AEntities::loopCount == 101)
        AEntities::loopCount = 0;
    AEntities::loopCount++;
    if (rand() % GameEngine::i < 5)
    {
        int enY = rand() % _winheight;
        addEntity(new Enemy(_win, _winwidth, (enY == 0) ? 1 : enY));
        if (GameEngine::i == EASY)
        {
            int asY = rand() % _winheight;
            addEntity(new Asteroids(_win, _winwidth, (asY == 0) ? 1 : asY));
        }
    }
    if (!updateEntities())
        return (false);
    drawEntities();
    refresh();
    return (true);
}

AEntities *         GameEngine::getEntity(int n) const
{
    return (_entities[n]);
}

void                GameEngine::drawEntities(void)
{
    _p1->draw();
    for (int i = 0; i < ENTITIES_MAX; i++)
    {
        if (_entities[i] != 0)
            _entities[i]->draw();
    }
}

void                GameEngine::addEntity(AEntities * entity)
{
    for (int i = 0; i < ENTITIES_MAX; i++)
    {
        if (_entities[i] == 0)
        {
            _entities[i] = entity;
            return ;
        }
    }
}

bool                GameEngine::updateEntities(void)
{
    _p1->del();
    _p1->update();
    addEntity(_p1->getMissile());
    // _p1->draw();
    for (int i = 0; i < ENTITIES_MAX; i++)
    {
        if (_entities[i])
        {
            _entities[i]->del();
            _entities[i]->update();
            // _entities[i]->draw();
            addEntity(_entities[i]->getMissile());
        }
    }
    if (playerColision())
        return (false);
    entityColision();
    return (true);
}

void                GameEngine::deleteEntity(AEntities * entity)
{
    for (int i = 0; i < ENTITIES_MAX; i++)
    {
        if ( _entities[i] == entity)
        {
            if (_entities[i])
                delete _entities[i];
            _entities[i] = 0;
            return ;
        }
    }
}

bool                GameEngine::playerColision(void)
{
    for (int i = 0; i < ENTITIES_MAX; i++)
    {
        if (_entities[i] && _p1->impact(_entities[i]))
            return (true);
    }
    return (false);
}

void                GameEngine::entityColision(void)
{
    for (int i = 0; i < ENTITIES_MAX; i++)
    {
        for (int j = i + 1; j < ENTITIES_MAX; j++)
        {
            if (_entities[i] && _entities[j] && _entities[i]->impact(_entities[j]))
            {
                _score += 50;
                deleteEntity(_entities[i]);
                deleteEntity(_entities[j]);
            }
        }
        if (_entities[i] && (_entities[i]->getX() < 0 || _entities[i]->getX() > 3000))
        {
            _score -= 10;
            deleteEntity(_entities[i]);
        }
    }
}

WINDOW const *      GameEngine::getWindow(void) const
{
    return (_win);
}

int         GameEngine::i = EASY;
int         GameEngine::_score = 0;

PS: The graphical part is handled with ncurses, but really, all there is to it is mvaddch(y, x, char), which draws a char at the specified position (yes, y first). Also, the GameEngine::i static thing for managing difficulty wasn't made by me, and it could indeed be improved by at least passing it to a GameEngine constructor.

Answer 1

One remark regarding the way collisions are handled:

bool playerColision(void);
void entityColision(void);

Basically what you want to do when you have a generic game engine is the possibilty to reuse your code, instead of having a distinction between "Player" and "AEntities" inside your GameEngine, your GameEngine could only handle "AEntities" entities and having a callback to call in the AEntity class

void OnCollision(AEntity* other);

This way each class could handle the collision the way they want.

Answer 2

Other answers have already covered some architectural aspects, so this review is going to be on coding style mostly.

1. Avoid exposing unnecessary dependencies in a header file. In the AEntities header file, you don't need <iostream>. Put that in the .cpps only if you need it.

2. Variables starting with an underscore are somewhat contrived in C++ land. Your usage of such naming is not conflicting with the standard, but I'd still suggest that you steer clear of this notation. Read this very good answer posted on StackOverflow for details.

3. Don't write void in the parameter list of a function that takes no arguments. Unlike in C, both T func(void); and T func(); mean the exact same thing in C++: a function taking 0 arguments. So adding void is pure boilerplate and adds nothing to the better understanding of the code.

4. Picking a little in your type naming, AEntities sounds wrong to me. The class is supposed to represent one entity, I figure, so why the plural name? And also, why the A prefix? Entity or GameEntity would read much more logically and nicer.

5. Again picking on naming, the variable that holds the game difficulty is called i? I mean, seriously? i is historically used as name for loop counters, using it for anything else is bound to generate confusion. Not to mention that in this case it is impossible to guess that i is actually the difficultyLevel. Can you change that? Please do so if you can. Also, a plain int seems deficient for it. Perhaps an enum would be better, then you could have predefined constants like Easy, Medium, Hard, Nightmare, etc...

6. In your implementation of GameEngine, what's with all the space between the return type and name of methods?

   bool                GameEngine::render(void)
   //  ^~~~~~~~~~~~~~~~ Why all this again?
   

   That's a huge waste of horizontal spacing.

7. return is not a function, so there is no reason to do add parentheses, besides the love for typing. I used to do this myself, now whenever I look at old code all I think is: why the heck did I do this everywhere?...

   return (true);
   //     ^    ^
   //     |    |
   //   don't need
   

8. The implementation of GameEngine::operator = seems very questionable. It only copies the _win member. What about the rest? But should a game instance be at all copyable? Perhaps you should take a look at The Rule of Three/Five/Zero and rethink this.

9. I see that you are using some raw pointers here and there. Manual memory management is a rare thing to see in user code nowadays. It is considered an outdated practice in face of standard containers and smart pointers. Memory erros make up for some of the hardest bugs to track down and memory leaks are very easy to introduce when doing manual allocations/deallocations. For your _entities array, a std::vector would be a cleaner choice and with it you could even lift the ENTITIES_MAX constraint (vector can grow dynamically).

10. You have a few "magic numbers" in entityColision(), such as 3000, 50 and 10. Hard to guess what they mean. Comments would help, named constants with descriptive names would be perfect.

Answer 3

I see some speedups but the optimizer probably already handles them. Still, it's good programming practice to move things out of loops when possible

I notice in entityColision() you delete an entity upon collision, which prevents it from participating in any other collisions. So, stop checking!

void                GameEngine::entityColision(void)
{
    for (int i = 0; i < ENTITIES_MAX-1; i++)
    {
        if (_entities[i])
        {
            for (int j = i + 1; j < ENTITIES_MAX; j++)
            {
               if (_entities[j] && _entities[i]->impact(_entities[j]))
               {
                   _score += 50;
                   deleteEntity(_entities,i);
                   deleteEntity(_entities,j);
                   goto NEXT_I; // no point checking any others
               }
            }
            int ix = _entities[i]->getX();
            if (ix < 0 || ix > 3000)
            {
                _score -= 10;
                deleteEntity(_entities,i);
            }
        }
NEXT_I:
    }
}

void                GameEngine::deleteEntity(AEntities * _entities, int i)
{
    if (_entities[i])
    {
        delete _entities[i];
        _entities[i] = 0;
    }
}

so you don't have to search for something when you already know where it is.