Snake game for Windows text console

Question

I have started to code Snake in C++. I am finding it hard to make progress on this project because the code is quite messy. How can I improve the structure of my program?

main.cpp:

#include <iostream>
#include "Game.h"
#define FRAMERATE 500
int main(void)
{
 Game game;
 int count = 0;
 while (game.program_is_running)
 {
 game.key_events();
 if (count >= FRAMERATE)
 {
 game.tick();
 count = 0;
 }
 else
 count++;
 }
 std::cout << "\nYou lose.\n";
 std::cin.get();
 return 0;
}

game.h:

#include <iostream>
#include <conio.h>
#include <stdlib.h>
#include <vector>
#include "Snake.h"
#define NUMBER_OF_ROWS 25
#define NUMBER_OF_COLUMNS 25
class Game
{
public:
 Game(void);
 void tick(void);
 void key_events(void);
 bool program_is_running;
private:
 char grid[NUMBER_OF_ROWS][NUMBER_OF_COLUMNS];
 Snake snake;
 Block apple;
 enum { right = 1, left, down, up };
 bool snake_is_touching_wall(void);
 void display_grid(void);
};

game.cpp:

#include <stdlib.h>
#include "Game.h"
Game::Game(void)
{
 for (int y = 0; y < NUMBER_OF_ROWS; y++)
 {
 for (int x = 0; x < NUMBER_OF_COLUMNS; x++)
 {
 if (y == 0 || y == NUMBER_OF_ROWS - 1)
 grid[y][x] = '#';
 else if (x == 0 || x == NUMBER_OF_COLUMNS - 1)
 grid[y][x] = '#';
 else
 grid[y][x] = ' ';
 }
 }
 apple.x = rand() % 25;
 apple.y = rand() % 25;
 program_is_running = true;
}
void Game::tick(void)
{
 for (int y = 0; y < NUMBER_OF_ROWS; y++)
 {
 for (int x = 0; x < NUMBER_OF_COLUMNS; x++)
 {
 if (y == 0 || y == NUMBER_OF_ROWS - 1)
 grid[y][x] = '#';
 else if (x == 0 || x == NUMBER_OF_COLUMNS - 1)
 grid[y][x] = '#';
 else
 grid[y][x] = ' ';
 }
 }
 std::vector<Block> snake_body = snake.get_body();
 if (snake.direction == right)
 {
 snake.move(0, 1);
 }
 else if (snake.direction == left)
 {
 snake.move(0, -1);
 }
 else if (snake.direction == down)
 {
 snake.move(1, 0);
 }
 else if (snake.direction == up)
 {
 snake.move(-1, 0);
 }
 for (int y = 0; y < NUMBER_OF_ROWS; y++)
 {
 for (int x = 0; x < NUMBER_OF_COLUMNS; x++)
 {
 if (y == apple.y && x == apple.x)
 grid[y][x] = '@';
 for (int i = 0; i < snake_body.size(); i++)
 {
 if (snake_body[i].y == y && snake_body[i].x == x)
 {
 grid[y][x] = snake.get_symbol();
 }
 }
 }
 }
 display_grid();
 if (snake_is_touching_wall())
 {
 program_is_running = false;
 }
}
void Game::key_events(void)
{
 char c;
 if (_kbhit())
 {
 c = _getch();
 switch (c)
 {
 case 'q':
 program_is_running = false;
 break;
 case 'l':
 if(snake.direction != left)
 snake.direction = right;
 break;
 case 'j':
 if(snake.direction != right)
 snake.direction = left;
 break;
 case 'k':
 if(snake.direction != up)
 snake.direction = down;
 break;
 case 'i':
 if(snake.direction != down)
 snake.direction = up;
 break;
 }
 }
}
bool Game::snake_is_touching_wall(void)
{
 std::vector<Block> snake_body = snake.get_body();
 const int SNAKE_HEAD_X = snake_body[0].x;
 const int SNAKE_HEAD_Y = snake_body[0].y;
 return grid[SNAKE_HEAD_Y][SNAKE_HEAD_X] == '#';
}
void Game::display_grid(void)
{
 system("cls");
 for (int y = 0; y < NUMBER_OF_ROWS; y++)
 {
 for (int x = 0; x < NUMBER_OF_COLUMNS; x++)
 {
 std::cout << grid[y][x] << ' ';
 }
 std::cout << std::endl;
 }
}

snake.h:

#include <vector>
struct Block
{
 int y, x;
};
class Snake
{
public:
 Snake();
 std::vector<Block> get_body();
 char get_symbol(void);
 void add_block(int, int);
 void move(int, int);
 int direction;
private:
 std::vector<Block> body;
 char symbol;
};

snake.cpp:

#include "Snake.h"
Snake::Snake(void)
{
 symbol = 'X';
 direction = 0;
 add_block(12, 12);
}
std::vector<Block> Snake::get_body()
{
 return body;
}
char Snake::get_symbol(void)
{
 return symbol;
}
void Snake::add_block(int y, int x)
{
 Block block;
 block.y = y;
 block.x = x;
 body.push_back(block);
}
void Snake::move(int y, int x)
{
 body[0].y += y;
 body[0].x += x;
}

Answer 1

You don’t use C++ classes to their fullest. Giving each class stuff to do, both encapsulates functionality and hides internal data structures. Looking at your Snake class: it is not much more than a data container, all snake functionality is encoded outside of the class.

For example:

std::vector<Block> get_body();

Here you are returning a copy of the internal data storage of the snake. Returning this means you want the caller to read where the snake is. Why not more specific functions that read where the head is, and if the snake is at a given (x,y) coordinates (this seems to be how you use the given information). (Of course, returning a copy where you could return a const reference is inefficient as well).

Another example:

void Snake::move(int y, int x)
{
 body[0].y += y;
 body[0].x += x;
}

Here you are implementing something that the Block class should be able to do. Wouldn’t it be clearer if this function was:

void Snake::move(Block shift)
{
 body[0] += shift;
}

Just create a operator+= for the Block class!

This function:

void Snake::add_block(int y, int x)
{
 Block block;
 block.y = y;
 block.x = x;
 body.push_back(block);
}

Should be:

void Snake::add_block(int y, int x)
{
 body.emplace_back(y,x);
}

Answer 2

C++ is not C

First problem is that you are writing everything like you were doing in C. Read about huge differences between languages. You should not write C++ code based on your experience in C.

Use constexpr

This is simply wrong, use constexpr.

#define FRAMERATE 500

should be

constexpr int FRAME_RATE {500};

Using void

You should not be putting void when there are no parameters.

char Snake::get_symbol(void)

should be

char Snake::get_symbol()

Enum

Modern C++ use class enum instead of simple enum You could read about it for example here. enum-vs-strongly-typed-enum

Hard-coding values

add_block(12, 12);

Try to give each number meaningful name, it will much more easier for readers to understand.

Duplicated Code

This code snippet is repeated twice. You should always avoid code duplication.

for (int y = 0; y < NUMBER_OF_ROWS; y++)
{
 for (int x = 0; x < NUMBER_OF_COLUMNS; x++)
 {
 if (y == 0 || y == NUMBER_OF_ROWS - 1)
 grid[y][x] = '#';
 else if (x == 0 || x == NUMBER_OF_COLUMNS - 1)
 grid[y][x] = '#';
 else
 grid[y][x] = ' ';
 }
}

Cyclomatic complexity

for in for in for and if This is too much. Try to avoid this kind of code, consider adding small functions with names that can explain what this code is doing.

for (int y = 0; y < NUMBER_OF_ROWS; y++)
{
 for (int x = 0; x < NUMBER_OF_COLUMNS; x++)
 {
 if (y == apple.y && x == apple.x)
 grid[y][x] = '@';
 for (int i = 0; i < snake_body.size(); i++)
 {
 if (snake_body[i].y == y && snake_body[i].x == x)
 {
 grid[y][x] = snake.get_symbol();
 }
 }
 }
}

• Mike Borkland: "Your naming convention is not popular among C++ developers." This should not be in the review, as it is completely opinion-based. In fact, Bjarne Stroustrup employs the same naming style as the OP and functions in the std namespace are all styled that way.
• Newbie: @Mike Borkland Thanks for commenting! I removed this from review.