39
\$\begingroup\$

This is my version of the Snake game, written in C++. How could it be improved, and what general advice would be useful for future projects?

#include <iostream>
#include <conio.h>

void run();
void printMap();
void initMap();
void move(int dx, int dy);
void update();
void changeDirection(char key);
void clearScreen();
void generateFood();

char getMapValue(int value);

// Map dimensions
const int mapwidth = 20;
const int mapheight = 20;

const int size = mapwidth * mapheight;

// The tile values for the map
int map[size];

// Snake head details
int headxpos;
int headypos;
int direction;

// Amount of food the snake has (How long the body is)
int food = 3;

// Determine if game is running
bool running;

int main()
{
    run();
    return 0;
}

// Main game function
void run()
{
    // Initialize the map
    initMap();
    running = true;
    while (running) {
        // If a key is pressed
        if (kbhit()) {
            // Change to direction determined by key pressed
            changeDirection(getch());
        }
        // Upate the map
        update();

        // Clear the screen
        clearScreen();

        // Print the map
        printMap();

        // wait 0.5 seconds
        _sleep(500);
    }

    // Print out game over text
    std::cout << "\t\t!!!Game over!" << std::endl << "\t\tYour score is: " << food;

    // Stop console from closing instantly
    std::cin.ignore();
}

// Changes snake direction from input
void changeDirection(char key) {
    /*
      W
    A + D
      S

      1
    4 + 2
      3
    */
    switch (key) {
    case 'w':
        if (direction != 2) direction = 0;
        break;
    case 'd':
        if (direction != 3) direction = 1;
        break;
    case 's':
        if (direction != 4) direction = 2;
        break;
    case 'a':
        if (direction != 5) direction = 3;
        break;
    }
}

// Moves snake head to new location
void move(int dx, int dy) {
    // determine new head position
    int newx = headxpos + dx;
    int newy = headypos + dy;

    // Check if there is food at location
    if (map[newx + newy * mapwidth] == -2) {
        // Increase food value (body length)
        food++;

        // Generate new food on map
        generateFood();
    }

    // Check location is free
    else if (map[newx + newy * mapwidth] != 0) {
        running = false;
    }

    // Move head to new location
    headxpos = newx;
    headypos = newy;
    map[headxpos + headypos * mapwidth] = food + 1;

}

// Clears screen
void clearScreen() {
    // Clear the screen
    system("cls");
}

// Generates new food on map
void generateFood() {
    int x = 0;
    int y = 0;
    do {
        // Generate random x and y values within the map
        x = rand() % (mapwidth - 2) + 1;
        y = rand() % (mapheight - 2) + 1;

        // If location is not free try again
    } while (map[x + y * mapwidth] != 0);

    // Place new food
    map[x + y * mapwidth] = -2;
}

// Updates the map
void update() {
    // Move in direction indicated
    switch (direction) {
    case 0: move(-1, 0);
        break;
    case 1: move(0, 1);
        break;
    case 2: move(1, 0);
        break;
    case 3: move(0, -1);
        break;
    }

    // Reduce snake values on map by 1
    for (int i = 0; i < size; i++) {
        if (map[i] > 0) map[i]--;
    }
}

// Initializes map
void initMap()
{
    // Places the initual head location in middle of map
    headxpos = mapwidth / 2;
    headypos = mapheight / 2;
    map[headxpos + headypos * mapwidth] = 1;

    // Places top and bottom walls 
    for (int x = 0; x < mapwidth; ++x) {
        map[x] = -1;
        map[x + (mapheight - 1) * mapwidth] = -1;
    }

    // Places left and right walls
    for (int y = 0; y < mapheight; y++) {
        map[0 + y * mapwidth] = -1;
        map[(mapwidth - 1) + y * mapwidth] = -1;
    }

    // Generates first food
    generateFood();
}

// Prints the map to console
void printMap()
{
    for (int x = 0; x < mapwidth; ++x) {
        for (int y = 0; y < mapheight; ++y) {
            // Prints the value at current x,y location
            std::cout << getMapValue(map[x + y * mapwidth]);
        }
        // Ends the line for next x value
        std::cout << std::endl;
    }
}

// Returns graphical character for display from map value
char getMapValue(int value)
{
    // Returns a part of snake body
    if (value > 0) return 'o';

    switch (value) {
        // Return wall
    case -1: return 'X';
        // Return food
    case -2: return 'O';
    }
}
\$\endgroup\$
  • \$\begingroup\$ Maybe you could have done it cross-plateforms also \$\endgroup\$ – Nicolas Charvoz Oct 13 '14 at 15:12
  • 1
    \$\begingroup\$ enum for direction instead of 1,2,3,4 and possibly a look up table for direction -> dx,dy \$\endgroup\$ – Karthik T Oct 14 '14 at 7:47
  • \$\begingroup\$ Add an emscripten build and host it on github pages or jsfiddle to play. \$\endgroup\$ – wtjones Oct 15 '14 at 1:07
  • 1
    \$\begingroup\$ FYI: interesting related answer on Stack Overflow \$\endgroup\$ – rolfl Oct 16 '14 at 21:45
  • \$\begingroup\$ Your game is awesome, I am learning from it :) \$\endgroup\$ – Damjan Pavlica Apr 28 '18 at 0:22
30
\$\begingroup\$

Unless you know how long the game loop will be on every computer, making your sleep a constant is generally bad practice. If you know that you want 2fps, a good way to keep it in line is get the time at the start of the game loop, then at the end, find out the difference, and use that to calculate the amount of time needed to sleep to keep the step the same. e.g, If the loop takes 0.1s, and you want 2fps, then put in the sleep of 0.4s.

Other than that, I'd possibly say that you need to have another variable alongside food which is snakeLength or something. I don't know if you're printing out the score on the screen, but if you are keeping track of the score, I'd imagine that you want it to start at 0, as opposed to 3, and 1 more int isn't that big a deal when you get better readability.

Possibly consider making direction an enum, with UP, DOWN, LEFT and RIGHT, because right now it's a little tricky to follow, and you wouldn't need to change too much of the logic, as enums are ints with some extra stuff, so you can easily compare in the way that you're doing now. Having said that, I'm not sure I follow what your direction values relate to, as I don't see anywhere that direction is set to 5, so that check seems unnecessary.

In your generateFood function, you access map directly, where you've made a function which does that job exactly in getMapValue, so you might want to consider using that, as at some point in the future you may decide to make it a Map class and then you'll run into errors with accessing private variables (I'd hope!).

Other than this, things seem pretty good, so I'm going to start nit-picking :P. I'd just suggest little things like alphabetizing your #includes and function prototypes. It's not that big a deal since you have 2, but something to bear in mind. As well as that, your clearScreen() and printMap() feels a lot like a Draw(), so you could possibly wrap them both up in that function, and just call init, update, draw and cleanup (when you're doing object loading and using pointers and whatnot) since you seem to be nearly following the game loop pattern (as an aside, if you are planning to make more games, read that entire book, it's a thing of beauty), and reading that article better explains my point about sleep.

\$\endgroup\$
21
\$\begingroup\$

This review is going to be mostly on code style and general code quality improvements.

OOP:

First consideration is that for a C++ program we expect to see some Object Oriented Programming - OOP. Your program is basically structured programming, which look a lot more like C than C++.

You should start out by refactoring the code into a few classes. Some classes like SnakeGame, Board/Map and Food come to mind.

Global variables:

Since you didn't use classes, you have ended up with some global variables in the file. Globals should be avoided in most cases. In your game, you could have passed those variables as arguments to the functions.

But suppose you want to stick with the globals. If so, you should still try to minimize scope. If the variables are not needed outside the file where they were declared, then you should wrap them inside an unnamed namespace, to make the variables file scoped. Like this:

namespace 
{
    // The tile values for the map
    int map[size];

    // Snake head details
    int headxpos;
    int headypos;
    int direction;

    ...
} // namespace

Now those variables cannot be accessed outside the file where they were declared. This reduces the chance of undesired state changes by other modules and makes debugging a little easier.

Also, it is preferable to always initialize global variables to some default safe value. These variables:

int headxpos;
int headypos;
int direction;
bool running;

Should be initialize to something.

Naming tidbits:

A preferred naming convention for constants, like mapwidth and mapheight, is ALL_UPPERCASE. This helps readability by clearly differentiating from mutable variables to compile-time constants.

const int MAP_WIDTH      = 20;
const int MAP_HEIGHT     = 20;
const int TOTAL_MAP_SIZE = MAP_WIDTH * MAP_HEIGHT;

Also, TOTAL_MAP_SIZE is more descriptive than just size.

Miscellaneous:

You could avoid the function prototypes by just placing main() at the end of the file.

getMapValue() could be simplified by replacing the if with a default case in the switch:

char getMapValue(int value)
{
    switch (value) {
    case -1 : return 'X'; // Return wall
    case -2 : return 'O'; // Return food
    default : return 'o'; // Returns a part of snake body
    }
}

Portability:

conio.h is unfortunately Windows only, so this code won't compile on other OSs. I don't know of any simple replacement to kbhit() and getch() but _sleep() can be replaced by std::this_thread::sleep_for() in C++11.

system("cls") is also not part of the C++ standard and therefore not portable.

\$\endgroup\$
  • 6
    \$\begingroup\$ Just my two cents, I see a lot of OOP in my workplace where it doesn't belong. Frankly, I think people should learn to program first, and then learn OOP, so I'd rank that suggestion much lower in priority than you. That being said, you aren't wrong, except in claiming that all C++ needs or expects to have OOP. \$\endgroup\$ – Poik Oct 13 '14 at 15:12
  • \$\begingroup\$ @Poik, by no means I claim that all C++ needs expect the use of OOP, that's why I used the phrase: "we expect to see some Object Oriented programming" as it is the main paradigm fostered by the language. In the code presented here, OOP would indeed provide a better solution. Just as pure functional programming also would. OOP should be used when it provides a benefit, not just because it is the "right way" of doing things. \$\endgroup\$ – glampert Oct 13 '14 at 16:31
  • 2
    \$\begingroup\$ I don't understand the anti-oop/pro-procedural argument. From an architecture perspective, a class (or whatever your language likes to call it) is just an instantiable version of what you're already writing. (your file's globals become member variables, and functions become methods). The biggest difference is OOP is more flexible, as interfaces can be defined and satisfied at runtime, enabling things like clean runtime implementation swapping or collections organized by interface instead of memory layout. \$\endgroup\$ – weberc2 Oct 13 '14 at 18:58
  • \$\begingroup\$ @Poik Sorry for marking it in C++ I was just going by this post link where the top answer states "if a person writes the code in a C style and compiles it with C++ then it is a C++ question". However I do see the OOP would make the code a lot easier to read and thank you for the tip :). \$\endgroup\$ – Funky Oct 13 '14 at 21:24
  • 2
    \$\begingroup\$ @weberc2 This isn't the forum for that unfortunately, but most of my knee-jerk is primarily from inheriting unreusable OOP code from people who don't understand the paradigm, but who use it only because they've always been told they should. That and as a high performance computing researcher, I tend to use a lot of procedural coding, since most things are, at least initially, ad hoc. Once I prove usefulness, then I try to abstract it out, or decide what is going to happen to it. Doing it the other way around would be much more work for me, but, as always, Your Mileage [Will] Vary. \$\endgroup\$ – Poik Oct 14 '14 at 14:11

protected by Jamal Jul 30 '15 at 5:04

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

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