Snake game in C++

Question

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';
    }
}

Answer 1

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.

Answer 2

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.