4
\$\begingroup\$

Recently, just for fun, I've created a clone of the arcade game Tron. Since I am still a novice when it comes to C++, I would really appreciate all the feedback I can get from those with more experience.

Cleanup.h

#ifndef TRON_CLEANUP_H
#define TRON_CLEANUP_H

#include <utility>
#include <SDL2/SDL.h>

/*
 * Recurse through the list of arguments to clean up, cleaning up
 * the first one in the list each iteration.
 */
template<typename T, typename... Args>
void cleanup(T *t, Args&&... args){
    //Cleanup the first item in the list
    cleanup(t);
    //Recurse to clean up the remaining arguments
    cleanup(std::forward<Args>(args)...);
}
/*
 * These specializations serve to free the passed argument and also provide the
 * base cases for the recursive call above, eg. when args is only a single item
 * one of the specializations below will be called by
 * cleanup(std::forward<Args>(args)...), ending the recursion
 * We also make it safe to pass nullptrs to handle situations where we
 * don't want to bother finding out which values failed to load (and thus are null)
 * but rather just want to clean everything up and let cleanup sort it out
 */
template<>
inline void cleanup<SDL_Window>(SDL_Window *win){
    if (!win){
        return;
    }
    SDL_DestroyWindow(win);
}
template<>
inline void cleanup<SDL_Renderer>(SDL_Renderer *ren){
    if (!ren){
        return;
    }
    SDL_DestroyRenderer(ren);
}
template<>
inline void cleanup<SDL_Texture>(SDL_Texture *tex){
    if (!tex){
        return;
    }
    SDL_DestroyTexture(tex);
}
template<>
inline void cleanup<SDL_Surface>(SDL_Surface *surf){
    if (!surf){
        return;
    }
    SDL_FreeSurface(surf);
}

#endif //TRON_CLEANUP_H
Contact GitHub API Training Shop Blog About

Controller.h

#ifndef TRON_CONTROLLER_H
#define TRON_CONTROLLER_H

#include "Includes.h"
#include "Player.h"

class Controller {
public:
    void control();

    Controller(Player *p1, Player *p2);
private:
    SDL_Event current_event;

    Player *p1_pointer = nullptr,
           *p2_pointer = nullptr;
};


#endif //TRON_CONTROLLER_H

Controller.cpp

#include "Controller.h"

Controller::Controller(Player *p1, Player *p2) : p1_pointer(p1), p2_pointer(p2) {}

void Controller::control() {
    SDL_PollEvent(&current_event);
    if (current_event.type == SDL_KEYDOWN) {
        switch(current_event.key.keysym.sym) {
            case SDLK_LEFT :
                switch (p1_pointer->getDirection()) {
                    case UP :
                        p1_pointer->changeDirection(LEFT);
                        break;
                    case DOWN :
                        p1_pointer->changeDirection(RIGHT);
                        break;
                    case LEFT :
                        p1_pointer->changeDirection(DOWN);
                        break;
                    case RIGHT :
                        p1_pointer->changeDirection(UP);
                        break;
                }
                break;
            case SDLK_RIGHT :
                switch (p1_pointer->getDirection()) {
                    case UP :
                        p1_pointer->changeDirection(RIGHT);
                        break;
                    case DOWN :
                        p1_pointer->changeDirection(LEFT);
                        break;
                    case LEFT :
                        p1_pointer->changeDirection(UP);
                        break;
                    case RIGHT :
                        p1_pointer->changeDirection(DOWN);
                        break;
                }
                break;
            case SDLK_a :
                switch (p2_pointer->getDirection()) {
                    case UP :
                        p2_pointer->changeDirection(LEFT);
                        break;
                    case DOWN :
                        p2_pointer->changeDirection(RIGHT);
                        break;
                    case LEFT :
                        p2_pointer->changeDirection(DOWN);
                        break;
                    case RIGHT :
                        p2_pointer->changeDirection(UP);
                        break;
                }
                break;
            case SDLK_d :
                switch (p1_pointer->getDirection()) {
                    case UP :
                        p2_pointer->changeDirection(RIGHT);
                        break;
                    case DOWN :
                        p2_pointer->changeDirection(LEFT);
                        break;
                    case LEFT :
                        p2_pointer->changeDirection(UP);
                        break;
                    case RIGHT :
                        p2_pointer->changeDirection(DOWN);
                        break;
                }
                break;
        }
    }
    else if (current_event.type == SDL_QUIT)
        exit(0);
}

Coord.h

#ifndef TRON_COORD_H
#define TRON_COORD_H

struct Coord {
    int x,
        y;
    Coord(int nx, int ny) : x(nx), y(ny) {}
    Coord() = default;
};

#endif //TRON_COORD_H

Field.h

#ifndef TRON_FIELD_H
#define TRON_FIELD_H

#include "Includes.h"

class Field {
public:
    static constexpr int SCREEN_WIDTH = 600;
    static constexpr int SCREEN_HEIGHT = 480;
    static constexpr short TILESIZE = 7;

    static constexpr int WIDTH = SCREEN_WIDTH / TILESIZE;
    static constexpr int HEIGHT = SCREEN_HEIGHT / TILESIZE;

    Field();
    Objects field_matrix[HEIGHT][WIDTH];
};


#endif //TRON_FIELD_H

Field.cpp

#include "Field.h"
#include <cstring>

Field::Field() {
    std::memset(field_matrix, NOTHING, sizeof(field_matrix[0][0]) * HEIGHT * WIDTH);
}

Includes.h

#ifndef TRON_INCLUDES_H
#define TRON_INCLUDES_H

#include <string>
#include <SDL2/SDL.h>
#include <SDL_mixer.h>
#include <SDL_ttf.h>
#include <SDL_image.h>
#include "Coord.h"

enum Dir {UP = 0, DOWN, LEFT, RIGHT};
enum Team {RED = 0, GREEN = 1};
enum Objects {NOTHING = 0, RED_TRAIL, PLAYER_1, GREEN_TRAIL, PLAYER_2};

#endif //TRON_INCLUDES_H

Model.h

#ifndef TRON_MODEL_H
#define TRON_MODEL_H

#include "Includes.h"
#include "Player.h"
#include "Trail.h"
#include "Field.h"
#include "Controller.h"
#include "View.h"

class Model {
public:
    void loop();
    void end_screen();

    Model();

    bool isGame_over_flag() const;

private:
    Player player_1,
           player_2;

    Field field;

    bool game_over_flag = false;

    Team winner;

    Controller controller;
    View view;

    constexpr static int max_fps = 60;
    int last_ticks = 0;
    int current_ticks;

    int movement_last_time = SDL_GetTicks();
    int movement_current_time;
    static constexpr int movement_wait_time = 50;

    int controls_last_time = SDL_GetTicks();
    int controls_current_time;
    static constexpr int controls_wait_time = 100;

    void update_matrix();
    void collision_detection(Player &p);
};


#endif //TRON_MODEL_H

Model.cpp

#include <iostream>
#include "Model.h"

Model::Model() : player_1(Field::WIDTH / 2 - Field::WIDTH / 4, Field::HEIGHT / 2, RED), player_2(Field::WIDTH / 2 + Field::WIDTH / 4, Field::WIDTH / 2, GREEN),
                 controller(&player_1, &player_2), view(&field)
{
    field.field_matrix[player_1.pos.y][player_1.pos.x] = PLAYER_1;
    field.field_matrix[player_2.pos.y][player_2.pos.x] = PLAYER_2;
}


void Model::loop() {
    controls_current_time = SDL_GetTicks();
    if ((controls_current_time - controls_last_time) > controls_wait_time) {
        controller.control();
        controls_last_time = controls_current_time;
    }

    movement_current_time = SDL_GetTicks();
    if ((movement_current_time - movement_last_time) > movement_wait_time) {
        update_matrix();
        movement_last_time = movement_current_time;
    }

    current_ticks = SDL_GetTicks();
    int delay = 1000 / max_fps - current_ticks + last_ticks;

    if(delay > 0)
        SDL_Delay(delay);

    view.draw();

    last_ticks = current_ticks;
}

void Model::end_screen() {
    view.draw_end_screen(winner);
}

bool Model::isGame_over_flag() const {
    return game_over_flag;
}

void Model::update_matrix() {
    Trail player1_new_trail = player_1.tick();
    field.field_matrix[player1_new_trail.pos.y][player1_new_trail.pos.x] = RED_TRAIL;
    collision_detection(player_1);
    field.field_matrix[player_1.pos.y][player_1.pos.x] = PLAYER_1;

    Trail player2_new_trail = player_2.tick();
    field.field_matrix[player2_new_trail.pos.y][player2_new_trail.pos.x] = GREEN_TRAIL;
    collision_detection(player_2);
    field.field_matrix[player_2.pos.y][player_2.pos.x] = PLAYER_2;
}

void Model::collision_detection(Player &p) {
    Team team = p.getTeam(),
         opponent_team;
    if (team == RED)
        opponent_team = GREEN;
    else if (team == GREEN)
        opponent_team = RED;

    if (field.field_matrix[p.pos.y][p.pos.x] == RED_TRAIL ||  field.field_matrix[p.pos.y][p.pos.x] == GREEN_TRAIL) {
        game_over_flag = true;
        winner = opponent_team;
        return;
    }

    if (p.pos.x < 0)
        p.pos.x = Field::WIDTH - 1;
    else if (p.pos.x >= Field::WIDTH)
        p.pos.x = 0;
    if (p.pos.y < 0)
        p.pos.y = Field::HEIGHT - 1;
    else if (p.pos.y >= Field::HEIGHT)
        p.pos.y = 0;
}

Player.h

#ifndef TRON_PLAYER_H
#define TRON_PLAYER_H

#include "Includes.h"
#include "Trail.h"

class Player {
public:
    void changeDirection(Dir new_dir);
    Dir getDirection() const;

    Team getTeam() const;

    Trail tick();

    Player() = default;
    Player(Coord pos, Team nteam);
    Player(int x, int y, Team nteam);

    Coord pos;
private:
    Dir direction = UP;
    Team team;
};


#endif //TRON_PLAYER_H

Player.cpp

#include "Player.h"

Player::Player(Coord npos, Team nteam) : team(nteam), pos(npos) {}

Player::Player(int x, int y, Team nteam) : team(nteam) {
    pos = Coord(x, y);
}

void Player::changeDirection(Dir newDir) {
    direction = newDir;
}

Dir Player::getDirection() const {
    return direction;
}

Trail Player::tick() {
    Trail new_trail(team, pos);
    switch(direction) {
        case UP :
            --pos.y;
            break;
        case DOWN :
            ++pos.y;
            break;
        case LEFT :
            --pos.x;
            break;
        case RIGHT :
            ++pos.x;
            break;
    }
    return new_trail;
}

Team Player::getTeam() const {
    return team;
}

Trail.h

#ifndef TRON_TRAIL_H
#define TRON_TRAIL_H

#include "Includes.h"

class Trail {
public:
    Team team;

    Coord pos;

    Trail(Team &team, int8_t x, int8_t y);
    Trail(Team &team, Coord pos);
};


#endif //TRON_TRAIL_H

Trail.cpp

#include "Trail.h"

Trail::Trail(Team &nteam, int8_t x, int8_t y) : team(nteam), pos(x, y) {}

Trail::Trail(Team &nteam, Coord npos) : team(nteam), pos(npos) {}

View.h

#ifndef TRON_VIEW_H
#define TRON_VIEW_H

#include "Includes.h"
#include "Field.h"
#include <SDL_ttf.h>
#include <memory>

class View {
public:
    View(Field *field);
    ~View();
    void draw();
    void draw_end_screen(Team Winner);
private:
    void logSDLError(const std::string &msg);
    void render_texture(SDL_Renderer *ren, SDL_Texture *tex, int x, int y, int w, int h);
    void render_texture(SDL_Renderer *ren, SDL_Texture *tex, int x, int y);
    SDL_Texture* render_font(const std::string &file, const std::string &msg, SDL_Color color, int font_size);

    SDL_Window *win = nullptr;
    SDL_Renderer *ren = nullptr;
    SDL_Texture *background = nullptr;

    static constexpr SDL_Color COLOR_RED = {255, 0, 0};
    static constexpr SDL_Color COLOR_GREEN = {0, 255, 0};

    Field *field_ptr = nullptr;
};

View.cpp

#include "View.h"
#include "Cleanup.h"
#include <iostream>

View::View(Field *field) : field_ptr(field) {
    if (SDL_Init(SDL_INIT_EVERYTHING) != 0) {
        std::cout << "Unable to initialise SDL: " << std::endl;
        exit(1);
    }

    if ((IMG_Init(IMG_INIT_PNG | IMG_INIT_JPG) & (IMG_INIT_PNG | IMG_INIT_JPG)) != (IMG_INIT_PNG | IMG_INIT_JPG)) {
        std::cout << "Unable to initialise SDL_img" << std::endl;
        SDL_Quit();
        exit(1);
    }

    if (TTF_Init() == -1) {
        std::cout << "Unable to initialise SDL_ttf" << std::endl;
        IMG_Quit();
        SDL_Quit();
        exit(1);
    }

    win = SDL_CreateWindow("Tron", SDL_WINDOWPOS_CENTERED, SDL_WINDOWPOS_CENTERED, field->SCREEN_WIDTH, field->SCREEN_HEIGHT, SDL_WINDOW_SHOWN);
    if (win == nullptr) {
        logSDLError("Window Initialisation");
        exit(1);
    }

    ren = SDL_CreateRenderer(win, -1, SDL_RENDERER_PRESENTVSYNC | SDL_RENDERER_ACCELERATED);
    if (ren == nullptr) {
        logSDLError("Renderer Initialisation");
        cleanup(win);
        IMG_Quit();
        SDL_Quit();
        exit(1);
    }

    background = IMG_LoadTexture(ren, "Resources/background.jpg");
    if (background == nullptr) {
        logSDLError("Texture");
        cleanup(ren, win);
        IMG_Quit();
        SDL_Quit();
        exit(1);
    }
}

View::~View() {
    cleanup(ren, win, background);
    TTF_Quit();
    IMG_Quit();
    SDL_Quit();
}

void View::draw() {
    // Render background texture
    render_texture(ren, background, 0, 0);

    SDL_Rect draw_dst;
    draw_dst.w = Field::TILESIZE;
    draw_dst.h = Field::TILESIZE;

    for (int y = 0; y < field_ptr->HEIGHT; ++y)
        for (int x = 0; x < field_ptr->WIDTH; ++x) {
            switch(field_ptr->field_matrix[y][x]) {
                case PLAYER_1 :
                case RED_TRAIL :
                    // Set the draw color
                    SDL_SetRenderDrawColor(ren, COLOR_RED.r, COLOR_RED.g, COLOR_RED.b, SDL_ALPHA_OPAQUE);

                    // Set the position to draw
                    draw_dst.x = x * Field::TILESIZE;
                    draw_dst.y = y * Field::TILESIZE;

                    // Draw
                    SDL_RenderFillRect(ren, &draw_dst);
                    break;
                case PLAYER_2 :
                case GREEN_TRAIL :
                    // Set the draw color
                    SDL_SetRenderDrawColor(ren, COLOR_GREEN.r, COLOR_GREEN.g, COLOR_GREEN.b, SDL_ALPHA_OPAQUE);

                    // Set the position to draw
                    draw_dst.x = x * Field::TILESIZE;
                    draw_dst.y = y * Field::TILESIZE;

                    // Draw
                    SDL_RenderFillRect(ren, &draw_dst);
                    break;
                case NOTHING :
                    break;
            }
        }

    SDL_RenderPresent(ren);
}

void View::draw_end_screen(Team winner) {
    std::string winner_text;
    SDL_Color winner_color;

    if (winner == RED) {
        winner_text = "Red Won";
        winner_color = {255, 0, 0};
    }
    else if (winner == GREEN) {
        winner_text = "Green Won";
        winner_color = {0, 255, 0};
    }
    else {
        winner_text = "I AM ERROR";
        winner_color = {255, 255, 255};
    }

    SDL_Texture *winner_texture = render_font("Resources/Pixelated.ttf", winner_text.c_str(), winner_color, 50);
    if (winner_texture == nullptr)
        exit(1);
    int w,
        h;
    SDL_QueryTexture(winner_texture, nullptr, nullptr, &w, &h);

    SDL_Rect dst;
    dst.x = Field::WIDTH / 2 + w / 2;
    dst.y = Field::HEIGHT / 2 + h / 2;
    dst.w = w;
    dst.h = h;

    SDL_RenderCopy(ren, winner_texture, nullptr, &dst);
    SDL_RenderPresent(ren);
}

void View::logSDLError(const std::string &msg) {
    std::cout << msg << " error: " << SDL_GetError() << std::endl;
}

void View::render_texture(SDL_Renderer *ren, SDL_Texture *tex, int x, int y, int w, int h) {
    SDL_Rect dst;
    dst.x = x;
    dst.y = y;
    dst.w = w;
    dst.h = h;
    SDL_RenderCopy(ren, tex, nullptr, &dst);
}

void View::render_texture(SDL_Renderer *ren, SDL_Texture *tex, int x, int y) {
    int w,
        h;
    SDL_QueryTexture(tex, nullptr, nullptr, &w, &h);
    render_texture(ren, tex, x, y, w, h);
}

SDL_Texture* View::render_font(const std::string &file, const std::string &msg, SDL_Color color, int font_size) {
    TTF_Font *font = TTF_OpenFont(file.c_str(), font_size);
    if (font == nullptr) {
        logSDLError("TTF Open Font");
        return nullptr;
    }

    SDL_Surface *temp_surface = TTF_RenderText_Blended(font, msg.c_str(), color);
    if (temp_surface == nullptr) {
        logSDLError("TTF Render Font");
        return nullptr;
    }

    SDL_Texture *tex = SDL_CreateTextureFromSurface(ren, temp_surface);
    if (tex == nullptr)
        logSDLError("Create Texture");

    TTF_CloseFont(font);
    SDL_FreeSurface(temp_surface);
    return tex;
}

Main.cpp

#include "Model.h"

int main() {
    Model model;
    while (!model.isGame_over_flag()) {
        model.loop();
    }
    model.end_screen();
    SDL_Delay(1000);
    return 0;
}
\$\endgroup\$
  • \$\begingroup\$ Please provide a reason for the downvotes \$\endgroup\$ – James Balajan Jan 3 '17 at 6:33
  • \$\begingroup\$ @Tushar I have included the code to my question \$\endgroup\$ – James Balajan Jan 3 '17 at 6:49
  • 1
    \$\begingroup\$ Although, you've included code in question, I'll suggest to only keep a part of the code which need to be reviewed. It'll be easy for reviewers. \$\endgroup\$ – Tushar Jan 3 '17 at 6:52
  • \$\begingroup\$ @Tushar I wish for the whole game to be reviewed if possible. That's why I included the github link because it is much easier to navigate and read, yet it seems the rules state the opposite, unfortunately. Is there anything else I can do to make your jobs easier? \$\endgroup\$ – James Balajan Jan 3 '17 at 6:58
  • 8
    \$\begingroup\$ It is fine to post a large amount of code/files in a post. It is also fine to include a link to the Github repository, for additional context if desired. The only down-side to posting a large amount of code is that it may take longer for you to receive answers, as it is more complicated to review. \$\endgroup\$ – Phrancis Jan 3 '17 at 7:03
8
+50
\$\begingroup\$

Couple of bugs!

File names typos

You seem to be using a case-insensitive OS. I've had to rename Resources/pixelated.ttf to Pixelated.ttf and cmake/FindSDL2_image.cmake to FindSDL2_Image.cmake.

Input issues

SDL_PollEvents only returns the first event waiting in the event queue. Thus, the code in Controller::control only handles one event per frame. For this reason, the game was at first completely unresponsive. I've quickly changed it to:

while(SDL_PollEvent(&current_event))
    if (current_event.type == SDL_KEYDOWN) {
        // Rest of the code...

Also, SDL KeySyms are good for text editing, but not great for game controls because they depend on the keyboard's layout. WASD in particular end up mapped to ZQSD on an AZERTY keyboard. Prefer scancodes for that.

The actual review

These sections are in no particular order, they are pretty much my notes while I'm reading through the various files and refactoring as I go.

The magic cleanup function!

I like what you've gone for in Cleanup.h, abstracting the various cleanup functions behind a single one. There are, however, a couple of things that can be improved.

Overload rather than specialize

M. Sutter has explained it here better than I ever could. In short: function template specializations act weird with overload resolution, so it's better to stick with plain overloads when they do the job:

inline void cleanup(SDL_Window *win) {
    if (!win) {
        return;
    }
    SDL_DestroyWindow(win);
}

// And so on.

Pack expansions are cool

Okay, they don't look that great in C++11. But it is definitely a good idea to get used to them, because C++17 brings along fold expressions, which makes the syntax really neat. In cases like this one, where you just want to repeat something on each argument of a pack, recursion with perfect forwarding is not your only option:

template<typename... Args>
void cleanup(Args *... args) {

    // C++11, constructing a dummy `int` array
    using ex = int[];
    (void)ex{(cleanup(args), 0)..., 0};

    // C++17
    (void)(cleanup(args), ...);
}

Can't clean this! Now what?

Now is a good time to think about the error case of our cleanup function. What happens when a developer lacking caffeine tries to call cleanup with something that doesn't have a suitable implementation ? Your original code produces this error output, with no hint towards the actual error:

In instantiation of ‘void cleanup(T*, Args&& ...) [with T = int; Args = {}]’:
  required from ‘void cleanup(T*, Args&& ...) [with T = int; Args = {SDL_Window*&}]’
  required from ‘void cleanup(T*, Args&& ...) [with T = SDL_Renderer; Args = {int*&, SDL_Window*&}]’
error: no matching function for call to ‘cleanup()’
     cleanup(std::forward<Args>(args)...);
            ^
note: candidate: template<class T, class ... Args> void cleanup(T*, Args&& ...)
 void cleanup(T *t, Args&&... args){
      ^
note:   template argument deduction/substitution failed:
note:   candidate expects at least 1 argument, 0 provided
     cleanup(std::forward<Args>(args)...);
            ^

Switching from specializations to overloads produces the same output. Using pack-expansion rather than recursion has... unfortunate results in this case: cleanup ends up calling itself recursively, compiling fine but blowing up the stack at runtime. Not ideal. To avoid both of these unpleasant symptoms of an error, we can declare a catch-all function which will be chosen whenever no version of cleanup can take in the argument.

template <class T>
void cleanup(T *) = delete;

Now it fails with a shorter and more helpful error:

error: use of deleted function ‘void cleanup(T*) [with T = int]’

This is enough for a trained developer to recognize the deleted catch-all pattern: he needs to implement cleanup(int*)! But we can do better:

template <class T>
struct zero {
    static constexpr int value = 0;
};

template <class T>
void cleanup(T *) {
    static_assert(zero<T>::value, "No overload of `cleanup` for this type.");
}

The zero<T>::value is here just to make sure the condition of static_assert depends on the template parameter, so it is only checked upon instantiating (trying to call) the function. Now the message appears clearly in the error log, guiding the developer towards solving the issue.

When to call cleanup?

So this cleanup function neatly reduces the syntax for cleaning up a couple of resources, being smart enough to know what to do depending on their types. But when do we actually call it? Turns out, C++ actually has a feature to know when to cleanup stuff: destructors! Rather than carefully calling cleanup at all the right places so nothing is left behind, even in the wildest cases of exception propagation, we can wrap every resource inside an object, whose destructor's job is to clean it up. This habit is known as RAII, probably the worst misnomer ever, but hey -- it's incredibly useful.

So the most visible resources are the window, the renderer and the background, three pointers with special cleanup requirements. This falls squarely into the functionality offered by std::unique_ptr, so let's use that. We just have to wrap cleanup inside a functor, and then declare some typedefs:

struct Cleaner {
    template <class T>
    void operator() (T *p) const {
        cleanup(p);
    }
};

using SdlWinPtr = std::unique_ptr<SDL_Window,   Cleaner>;
using SdlRenPtr = std::unique_ptr<SDL_Renderer, Cleaner>;
using SdlTexPtr = std::unique_ptr<SDL_Surface,  Cleaner>;

Then later I stumbled upon some owning SDL_Surface and TTF_Font pointers, so I've added them too. This led to refactoring View::render_font, which returned an owning SDL_Texture*, which had to be cleaned up by the caller later. And guess what -- it wasn't done. Replacing the return type with SdlTexPtr fixed that bug for free :)

Clean ALL the things!

Now we can replace our raw owning pointers with these, and let them handle all of the cleaning for us. But wait! There's still cleanup code all over the place inside View.cpp, that is TTF_Quit(), IMG_Quit() and SDL_Quit(). Libraries are a resource too, so let's wrap them in some nice little RAII classes.

// Helper to avoid deleting functions all over the place
struct NoCopy {
    NoCopy() = default;
    NoCopy(NoCopy const &) = delete;
    NoCopy &operator = (NoCopy const &) = delete;
};

struct SdlHandle : NoCopy {
    SdlHandle(Uint32 flags) {
        if(SDL_Init(flags))
            throw std::runtime_error("Unable to initialise SDL");
    }

    ~SdlHandle() { SDL_Quit(); }
};

struct ImgHandle : NoCopy {
    ImgHandle(int flags) {
        if(IMG_Init(flags) != flags)
            throw std::runtime_error("Unable to initialise SDL_img");
    }

    ~ImgHandle() { IMG_Quit(); }
};

struct TtfHandle : NoCopy {
    TtfHandle() {
        if(TTF_Init() == -1)
            throw std::runtime_error("Unable to initialise SDL_ttf");
    }

    ~TtfHandle() { TTF_Quit(); }
};

Exceptions go hand-in-hand with RAII -- and are in fact part of the explanation for this bizarre abbreviation. Resource Acquisition Is Initialization means that the lifetime of a resource is tied to the lifetime of an object. When the object is initialised, the resource is acquired. If the resource cannot be acquired, the object must fail to construct: this is done by throwing an exception.

Oh, by the way, did you spot the bug? You had forgotten to call TTF_Quit() if the initialization inside View's constructor failed ;)

But let's integrate these three handles into View, so they are constructed and destructed automatically with it. We can do this either by placing them as first members, or as base classes. The latter stretches a bit what base classes were intended for, but avoids creating a name that won't be used otherwise, so I'll do that. YMMV.

class View : private SdlHandle, ImgHandle, TtfHandle {
    // ...

And the constructor becomes:

View::View(Field *field)
: SdlHandle(SDL_INIT_EVERYTHING)
, ImgHandle(IMG_INIT_PNG | IMG_INIT_JPG)
, field_ptr(field) {
    // All is setup!
    // ...

And the destructor becomes... nothing! We have "accidentally" met the Rule of Zero: now that all of our resources are managed by autonomous objects and we don't have to juggle error conditions, we don't have to implement a destructor. Note that we also lost the copy- and move- constructors and assignment operators. These were originally available for View, but were meaningless -- if you ended up calling them, you were in for a bunch of bugs, ending with Undefined Behaviour from double deletion. Now they're implicitly deleted, because those from the Handles are too. Good riddance!

std::exit with a bang

From cppreference:

Stack is not unwound: destructors of variables with automatic storage duration are not called.

While not as drastic as std::abort, std::exit is essentially a self-destruct function. It's only to be used when nothing can be done anymore to recovere from an error, and you have to make your program blow up on the spot. In more civilised cases, prefer throwing exceptions.

logSDLError has interesting functionality, retrieving the error message from SDL. Let's mix this with the above recommendation:

[[noreturn]] void throwSdlError(std::string const &msg) {
    throw std::runtime_error(msg + " error: " + SDL_GetError());
}

And now View's constructor (and some other functions) looks like this:

    // ...
    win.reset(SDL_CreateWindow(/* ... */));
    if (!win)
        throwSdlError("Window Initialisation");

    ren.reset(SDL_CreateRenderer(/* ... */));
    if (!ren)
        throwSdlError("Renderer Initialisation");

    background.reset(IMG_LoadTexture(/* ... */));
    if (!background)
        throwSdlError("Texture");
}

Sowing zeroes

Let's look at Field's constructor:

Field::Field() {
    std::memset(field_matrix, NOTHING, sizeof(field_matrix[0][0]) * HEIGHT * WIDTH);
}

There's a subtle, not-yet-a-bug hidden in there.

  • Objects is a plain enum with no fixed underlying type, which means that it's at least an int (4 bytes on my machine);
  • std::memset takes in an unsigned char value.

For now, since the default value is NOTHING, which itself is defined as 0, std::memset fills the array with zero-valued bytes which end up forming coherent NOTHING's. But if you swap the Objects around inside their enum, or decide to initialise with something else, std::memset will chop off three bytes out of the value, and essentially quadruple the remaining byte, so 1 becomes 16843009. This is not going to end well.

So, prefer std::fill_n:

std::fill_n(&field_matrix[0][0], WIDTH * HEIGHT, NOTHING);

Freeing the functions

Both overloads of View::render_texture have no dependency on View's members, so they can be made static. This is also almost the case for render_font: it only depends on the renderer, which is given to both other functions through a parameter. Let's iron that difference out and have all three be static.

But why stop here? These functions are also private: they are pure implementation details -- let's go one step further and remove them from View.h completely. Now that they are static, this is easy: just remove their declarations, and wrap them in an anonymous namespace inside View.cpp.

namespace {
    void render_texture(SDL_Renderer *ren, SDL_Texture *tex, int x, int y, int w, int h) {/* ... */}

    void render_texture(SDL_Renderer *ren, SDL_Texture *tex, int x, int y) {/* ... */}

    SdlTexPtr render_font(SDL_Renderer *ren, const std::string &file, const std::string &msg, SDL_Color color, int font_size) {/* ... */}
}

Landmine debugging

In View::draw_end_screen there's a peculiar sight: if the function is called without a winner, "I AM ERROR" is displayed instead. Really the function should either handle the case in a sensible way (displaying "Tie" for example), or do some actual error reporting. In its current state, the game doesn't handle ties: if both players collide head-on, Red wins. In fact, the Team enum only has RED and GREEN, so a Team variable holding a value that is neither RED nor GREEN is really fishy. So let's consider reporting an error.

Unlike the various possible resource loading errors from earlier, ending up displaying the Game Over screen without a winner is not an exceptional, but expectable condition: it's a bug. If this happens, then the program is broken -- please attach a debugger and dive in! That is what the assert function -- a macro, really -- is for: assert what has to be true, and it will diligently blow up in your face if reality turns out to be wrong. Additionally, since this is for debugging purposes only, all asserts will vanish if you compile your release version with -NDEBUG.

Adding assert to the simple case of draw_end_screen leads to the code below. Do feel free to pepper the your code with asserts whenever a non-obvious operation has to leave something in a specific state -- the more there are, the sooner bugs are caught. They're a bit like built-in unit tests, in a way.

void View::draw_end_screen(Team winner) {
    // ...
    if (winner == RED) {
        // ...
    }
    else if (winner == GREEN) {
        // ...
    }
    else {
        assert(!"draw_end_screen called with no winner");
    }
    // ...

... okay, there's a little lie above. Quoting cppreference about converting to an enum:

The result is unspecified if the value, converted to the enumeration's underlying type, is out of this enumeration's range.

... and the "unspecified" turns into "undefined behaviour" come C++17. So unfortunately, you can't really test whether something broke your enum's value -- the Standard just says "don't break your enum's value". But do use assert when you're not already toasted ;)

Splitting up concerns

Let's have a look at the Controller::control function. As it is now, it contains a handful of things:

  • The events consumption
  • The reaction to the quit event
  • The key mapping for both players
  • The players' rotation logic

That last bullet point looks strongly out-of-place -- player physics in input-handling code ? This should definitely be somewhere with the Player class instead, as is further hinted at by the code repetition: both halves of the input handling code are completely identical, except for which player they operate on. In fact, my pinky tells me you've copied and pasted it -- or maybe it's the bug on line 62, where you've forgotten to replace p1 with p2 ;)

So let's extract this code and put it inside member functions of Player:

void Player::turnLeft() {
    switch (getDirection()) {
        case UP:    changeDirection(LEFT); break;
        case DOWN:  changeDirection(RIGHT); break;
        case LEFT:  changeDirection(DOWN); break;
        case RIGHT: changeDirection(UP); break;
    }
}

Player::turnRight is as you'd expect. Now the input handling looks like this:

if (current_event.type == SDL_KEYDOWN) {
    switch(current_event.key.keysym.sym) {
        case SDLK_LEFT:  p1_pointer->turnLeft(); break;
        case SDLK_RIGHT: p1_pointer->turnRight(); break;
        case SDLK_a:     p2_pointer->turnLeft(); break;
        case SDLK_d:     p2_pointer->turnRight(); break;
    }
}

While we're looking at event handling, there's no reason for the SDL_event to be kept as a member -- it is overwritten by SDL_PollEvent, so a local variable is fine.

Quitting cleanly

As mentioned above, the std::exit function brings your program to quite a brutal stop. Let's find a cleaner way to handle the SDL_QUIT event from Controller::control: Model already takes care of one exit condition, that is the end-of-game state. Controller does not know Model, but it already knows both of its Players, so changing this isn't a big stretch. So Controller now holds a Model*, and Model acquires three new functions, player1(), player2() and quit(). A bit more logic to distinguish between a player win and a quit event, and avoid displaying the winner screen in the latter case, and we can exit cleanly!

Splitting up more concerns

Model::collision_detection has several problems:

  • For unknown reasons, it handles not only the collision detection, but also the wrapping of the player across the field.
  • The wrapping is handled last... but if the player has to wrap, this means he is outside the field, and all of the previous code has worked with out-of bound coordinates, while the player was in an invalid state.

To solve this, we'll move the wrapping code together with the Player movement code inside tick(). That way, the player handles its own motion, wrapping included, and cannot be left outside the field.

While I'm looking at the Player class: you really don't want that default constructor -- that would construct a player whose team attribute's value is indeterminate. Using it anywhere would then be UB. Same remark for the default constructor of Coords, which doesn't initialise the coordinates.

Code repetition

Model::update_matrix can be very quickly de-duplicated by having the same structure as collision_detection: acting on a single player, and getting called twice.

Unused stuff

There's a couple of things that aren't used anywhere:

  • Trail::team. It's initialised, but never read. That makes Trail itself a bit redundant, as only the position is used. Making Player::tick(), the only use of Trail, return a Coord instead works fine.
  • The (Coord, Team) overload of Player's constructor.

Adjusting names

Naming is important, so here are some renaming suggestions:

  • Controller::controlController::handleEvents (signaling that it empties the SDL event queue)
  • Field::field_matrixField::matrix (no need to repeat oneself)
  • Model::update_matrixModel::updatePlayer
  • Model::isGame_over_flagModel::isGameOver
  • Model::collision_detectionModel::checkCollision (renamed to a verb)
  • Player::tickPlayer::step (slightly more descriptive)
  • View::field_ptrView::field (no need to repeat the type in the variable's name)

The top-level design

You visibly intend to follow the Model-View-Controller pattern. There are, however, a few points that puzzle me. As I am not really familiar with the MVC pattern myself, I will leave these as open questions.

Polymorphism?

One of the big arguments I heard in favour of MVC is the ability to swap out its different pieces, particularly the view. In your code, the View is a concrete class, whose instance is allocated directly inside the Model object. It cannot be replaced.

Why does Model contain everything?

I was a bit surprised to see only Model inside main: everything else, including the View and the Controller, is inside. Usual MVC implementation instanciate the three parts in the same place (e.g. in main) and wire them together before running the program.

Is MVC actually adequate?

Unless the goal of this program is to learn about MVC, of course. But in its current state, it looks like MVC doesn't bring much to the table.

The Controller, once pruned from the physics code that sneaked in, is no more than a void handleEvents(Model *) function bent into a class. Which, in turn, looks closely like a Model::handleEvents() function.

The Model itself is a bit crowded already. Apart from the View and Controller, it contains the players and field, the state of the game, the FPS capping, and a pretty complicated bit of timing logic (which is also duplicated between the "controls" and the "movement").
It looks like everything that should, according to MVC, go "in the Model" litterally went into the Model class. Maybe what you should do is design your classes independently, then group them inside a model namespace.

Wrapping up!

Well, I hope I didn't ramble too far. That is a pretty long review (okay, that is a huge review), but with any luck it will actually be useful! Do let me know if something is unclear, incomplete, or anything else.

Happy coding :)

\$\endgroup\$
  • \$\begingroup\$ Looks pretty comprehensive! I'll post the parts of my own partially completed review once I remove the portions you've already covered. \$\endgroup\$ – Edward Jan 8 '17 at 1:33
  • \$\begingroup\$ Is MVC adequate? It could be. If it was loosely coupled he could replace Controller and have an A.I. driven simulation for machine learning exercises, etc. \$\endgroup\$ – cprn Jan 8 '17 at 3:13
  • 1
    \$\begingroup\$ I didn't notice the little comic linked in the exclamation mark until now. This answer is so full of personality I love it! \$\endgroup\$ – James Balajan Jan 8 '17 at 8:06
  • 1
    \$\begingroup\$ @JamesBalajan I'm very glad you like it, I was a bit afraid of having gone too verbose :) \$\endgroup\$ – Quentin Jan 8 '17 at 19:00
  • 1
    \$\begingroup\$ @JamesBalajan Of course! I honestly can't be bothered with a pull request, so here's a zip file :p wyvup.com/?c=A2VWloW \$\endgroup\$ – Quentin Jan 9 '17 at 18:05

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.