This is what I came up with when trying to make my first console game. I think it turned out quite okay so I wanted to know what you think about it. If there is anything to improve, may it be coding style, readability, efficiency or anything else, please let me know.
main.cpp
#include <iostream>
#include <stdlib.h>
#include <conio.h>
#include <windows.h>
#include "game.h"
int main()
{
bool exit = false;
std::cout << "Controls:\n"
<< " - Use w, a, s, d to change direction.\n"
<< " - Press space to pause the game.\n\n";
system("pause");
system("cls");
while( !exit )
{
size_t hight = 20, width = 20;
Game game(hight , width);
game.StartGame();
while( !game.isGameOver )
{
game.Update();
Sleep( 100 );
}
std::cout << "\nGame Over.\nPress 1 to play again.\nPress Esc to exit.";
char input = 0;
while( input != '1' && input != 27 )
{
input = getch();
}
if( input == 27)
exit = true;
system("cls");
}
return 0;
}
field.h
#ifndef FIELD_H
#define FIELD_H
#include <iostream>
#include <conio.h>
class Field
{
public:
// Constructor & Destructor
Field( size_t hight, size_t width );
~Field();
// Methods
void Draw();
// Members
char **field;
enum BlockType{ EMPTY = ' ', WALL = '#', FOOD = '*', sHEAD = 2, sBODY = 'o' };
private:
size_t HIGHT;
size_t WIDTH;
};
#endif // FIELD_H
field.cpp
#include "field.h"
Field::Field( size_t hight, size_t width )
: field( nullptr ), HIGHT( hight ), WIDTH( width )
{
// allocate field array
field = new char* [HIGHT];
for( size_t i = 0; i < HIGHT; i++)
{
field[i] = new char[WIDTH];
}
// initialize field array
for( size_t i = 0; i < WIDTH; i++ )
field[0][i] = field[HIGHT - 1][i] = WALL; // first and last line are borders
for( size_t i = 1; i < HIGHT - 1; i++ )
{
field[i][0] = field[i][WIDTH - 1] = WALL;
for( size_t j = 1; j < WIDTH - 1; j++ ) // first and last column are borders
{
field[i][j] = ' ';
}
}
}
Field::~Field()
{
for( size_t i = 0; i < HIGHT; i++)
{
delete [] field[i];
}
delete [] field;
}
void Field::Draw()
{
for( size_t i = 0; i < HIGHT; i++)
{
for( size_t j = 0; j < WIDTH; j++)
{
putch(field[i][j]);
}
putch('\n');
}
}
snake.h
#ifndef SNAKE_H
#define SNAKE_H
#include <iostream>
class Snake
{
public:
// Constructor & Destructor
Snake( size_t MaxLength, size_t pos_x, size_t pos_y );
~Snake();
// Members
enum Direction{ UP = 'w', DOWN = 's', LEFT = 'a', RIGHT = 'd', UNDIRECTED };
size_t **snake;
size_t length;
Direction direction;
size_t temp_TailPosition[2];
// Methods
void ChangeDirection( Direction dir );
void Move();
void Grow();
private:
size_t maxLength;
};
#endif // SNAKE_H
snake.cpp
#include "snake.h"
Snake::Snake( size_t MaxLength, size_t pos_x, size_t pos_y )
: length( 3 ), direction( UNDIRECTED ), maxLength( MaxLength )
{
// Allocate snake array
snake = new size_t *[maxLength];
for( size_t i = 0; i < maxLength; i++)
{
snake[i] = new size_t [2];
}
// Initialize snake array
for( size_t i = 0; i < length; i++)
{
snake[i][0] = pos_x;
snake[i][1] = pos_y;
}
// Initialize temp_TailPosition
temp_TailPosition[0] = pos_x;
temp_TailPosition[1] = pos_y;
}
Snake::~Snake()
{
for( size_t i = 0; i < maxLength; i++)
{
delete [] snake[i];
}
delete [] snake;
}
void Snake::ChangeDirection( Direction dir )
{
switch( dir )
{
case UP:
if( direction != DOWN )
direction = UP;
break;
case DOWN:
if( direction != UP )
direction = DOWN;
break;
case LEFT:
if( direction != RIGHT )
direction = LEFT;
break;
case RIGHT:
if( direction != LEFT )
direction = RIGHT;
break;
case UNDIRECTED:
break;
}
}
void Snake::Move()
{
if( direction == UNDIRECTED )
return;
// Remember tail position
temp_TailPosition[0] = snake[length - 1][0];
temp_TailPosition[1] = snake[length - 1][1];
// Move Body
for( size_t i = length - 1; i > 0; i-- )
{
snake[i][0] = snake[i - 1][0];
snake[i][1] = snake[i - 1][1];
}
// Move Head
switch( direction )
{
case UP:
snake[0][1] -= 1;
break;
case DOWN:
snake[0][1] += 1;
break;
case LEFT:
snake[0][0] -= 1;
break;
case RIGHT:
snake[0][0] += 1;
break;
}
}
void Snake::Grow()
{
length++;
snake[length - 1][0] = temp_TailPosition[0];
snake[length - 1][1] = temp_TailPosition[1];
}
game.h
#ifndef GAME_H
#define GAME_H
#include <iostream>
#include <conio.h>
#include <windows.h>
#include <stdlib.h> // srand, rand
#include <time.h> // time
#include "snake.h"
#include "field.h"
class Game
{
public:
Game( size_t hight, size_t width );
// Members
bool isGameOver;
// Methods
void StartGame();
char GetInput();
Snake::Direction GetDirection( char input );
void Update();
void Draw();
private:
// Members
Field FIELD;
Snake SNAKE;
size_t HIGHT;
size_t WIDTH;
size_t score;
// Methods
void SpawnFood();
void PutSnake();
void MoveCurser( size_t x, size_t y );
void GameOver();
void Pause();
};
#endif // GAME_H
game.cpp
#include "game.h"
Game::Game( size_t hight, size_t width )
: isGameOver( false ),
FIELD( hight, width ),
SNAKE( hight * width, width / 2, hight / 2),
HIGHT( hight ),
WIDTH( width ),
score( 0 )
{
// Put Head
size_t &headPos_x = SNAKE.snake[0][0];
size_t &headPos_y = SNAKE.snake[0][1];
FIELD.field[headPos_y][headPos_x] = Field::BlockType::sHEAD;
// Generate random seed
srand(time(NULL));
// Spawn Food
SpawnFood();
}
void Game::StartGame()
{
Draw();
char input = 0;
while( input != 'w' && input != 'a' && input != 's' && input != 'd' )
{
input = GetInput();
}
Snake::Direction direction = GetDirection( input );
SNAKE.ChangeDirection( direction );
SNAKE.Move();
PutSnake();
Draw();
}
char Game::GetInput()
{
if( kbhit() == true )
{
char input = getch();
return input;
}
return 0;
}
Snake::Direction Game::GetDirection( char input )
{
switch(input)
{
case 'w':
return Snake::Direction::UP;
case 'a':
return Snake::Direction::LEFT;
case 's':
return Snake::Direction::DOWN;
case 'd':
return Snake::Direction::RIGHT;
default:
return Snake::Direction::UNDIRECTED;
}
}
void Game::Update()
{
char input = GetInput();
if( input == ' ' )
Pause();
Snake::Direction direction = GetDirection( input );
SNAKE.ChangeDirection( direction );
SNAKE.Move();
PutSnake();
Draw();
}
void Game::Draw()
{
MoveCurser( 0, 0 );
FIELD.Draw();
std::cout << "Score: " << score;
}
void Game::SpawnFood()
{
size_t x = 0,
y = 0;
while( FIELD.field[y][x] != Field::BlockType::EMPTY )
{
x = rand() % WIDTH;
y = rand() % HIGHT;
}
FIELD.field[y][x] = Field::BlockType::FOOD;
}
void Game::PutSnake()
{
size_t &headPos_x = SNAKE.snake[0][0];
size_t &headPos_y = SNAKE.snake[0][1];
size_t &oldTailPos_x = SNAKE.temp_TailPosition[0];
size_t &oldTailPos_y = SNAKE.temp_TailPosition[1];
switch( FIELD.field[headPos_y][headPos_x])
{
case Field::BlockType::FOOD:
SNAKE.Grow();
score++;
FIELD.field[headPos_y][headPos_x] = Field::BlockType::sHEAD; // Put Head
for( size_t i = 1; i < SNAKE.length; i++ ) // Put Body
{
FIELD.field[SNAKE.snake[i][1]][SNAKE.snake[i][0]] = Field::BlockType::sBODY;
}
SpawnFood();
break;
case Field::BlockType::EMPTY:
FIELD.field[oldTailPos_y][oldTailPos_x] = Field::BlockType::EMPTY; // Remove old Tail
FIELD.field[headPos_y][headPos_x] = Field::BlockType::sHEAD; // Put Head
for( size_t i = 1; i < SNAKE.length; i++ ) // Put Body
{
FIELD.field[SNAKE.snake[i][1]][SNAKE.snake[i][0]] = Field::BlockType::sBODY;
}
break;
case Field::BlockType::WALL:
case Field::BlockType::sBODY:
isGameOver = true;
GameOver();
break;
}
}
void Game::MoveCurser( size_t x, size_t y )
{
HANDLE hOut;
COORD position;
hOut = GetStdHandle( STD_OUTPUT_HANDLE );
position.X = x;
position.Y = y;
SetConsoleCursorPosition( hOut, position);
}
void Game::GameOver()
{
size_t &oldHeadPos_x = SNAKE.snake[1][0];
size_t &oldHeadPos_y = SNAKE.snake[1][1];
for( char i = 0; i < 4; i++ )
{
MoveCurser( oldHeadPos_x, oldHeadPos_y);
putch(Field::BlockType::sHEAD);
Sleep(150);
MoveCurser( oldHeadPos_x, oldHeadPos_y);
putch(Field::BlockType::EMPTY);
Sleep(150);
}
}
void Game::Pause()
{
system("cls");
std::cout << "Game paused.\nPress space to continue.";
char input = 0;
while( input != ' ')
{
input = getch();
}
system("cls");
Draw();
}
3 Answers 3
I have found a couple of things that could help you improve your code.
Don't use system("cls")
There are two reasons not to use system("cls")
or system("pause")
. The first is that it is not portable to other operating systems which you may or may not care about now. The second is that it's a security hole, which you absolutely must care about. Specifically, if some program is defined and named cls
or pause
, your program will execute that program instead of what you intend, and that other program could be anything. First, isolate these into a seperate functions cls()
and pause()
and then modify your code to call those functions instead of system
. Then rewrite the contents of those functions to do what you want using C++. For example, if your terminal supports ANSI Escape sequences, you could use this:
void cls()
{
std::cout << "\x1b[2J";
}
Isolate platform-specific code
In this code, there are several things that are DOS/Windows only including #include <conio.h>
and the getch()
and kbhit()
functions within that, and also system("cls");
that I've already mentioned. Your code runs successfully on Linux if I supply those missing functions, but it would be nice if there were an #ifdef WINDOWS
already in the code so that one could recompile without having to alter the source code.
Consider using a better random number generator
You are currently using
x = rand() % WIDTH;
There are problems with this approach. One is that the low order bits of the random number generator are not particularly random, so neither is x
. On my machine, there's a slight but measurable bias toward 0 with that. A better solution, if your compiler and library supports it, would be to use the C++11 `std::uniform_int_distribution. It looks complex, but it's actually pretty easy to use. Here's an example:
unsigned rand_uint(unsigned low, unsigned high)
{
static std::default_random_engine re{};
using Dist = std::uniform_int_distribution<unsigned>;
static Dist uid{};
return uid(re, Dist::param_type{low,high});
}
Eliminate "magic numbers"
There are a few numbers in the code, such as 20
and 27
that have a specific meaning in their particular context. By using named constants such as WIDTH
or ESC
, the program becomes easier to read and maintain.
Use correct spellings
The word "height" is misspelled as "hight" throughout the program. It's not a major point but it makes your code harder for others to use and understand if common words are not spelled correctly.
Use const
where practical
The hight
and width
variables in main.cpp
are never altered in the program and should be declared as const
. Similarly, member functions that don't alter the underlying object, such as Field::Draw()
should be declared as const
.
Separate class responsibilities more completely
The Game
class reaches into the Snake
class and directly manipulates Snake
member variables. This is a sign that the classes are not as well designed as they could be. Specifically, it seems that much of the code that is currently in Game::PutSnake()
should be delegated instead to the Snake
object. To take another specific example, consider rewriting the Game::SpawnFood()
function to eliminate the need for direct access to the field
member data:
void Game::SpawnFood()
{
size_t x = 0,
y = 0;
while( !FIELD.isEmpty(x,y) )
{
x = rand_uint(0, WIDTH);
y = rand_uint(0, HIGHT);
}
FIELD.dropFood(x, y);
}
Carefully separate interface from implementation
The #include
files that you put into a .h
file are a part of its interface, while the ones in the corresponding .cpp
file are part of the implementation. So for instance, conio.h
is not required to understand and use the interface for Field
and so that #include
should not appear in the field.h
file. It should instead be put into the field.cpp
file since it's a detail of the implementation and not of the interface.
Omit return 0
When a C++ program reaches the end of main
the compiler will automatically generate code to return 0, so there is no reason to put return 0;
explicitly at the end of main
.
-
3\$\begingroup\$ Minor quibble: the reason for putting
return 0;
at the end ofmain
is that without it the code looks wrong. <g> It's a weird special case, intended to make it easier to teach absolute beginners. But part of what it teaches has to be unlearned when writing any other value-returning function. Personally, I always put it inmain
. But I don't disagree with people who leave it out. \$\endgroup\$Pete Becker– Pete Becker2015年07月13日 16:42:39 +00:00Commented Jul 13, 2015 at 16:42 -
\$\begingroup\$ @PeteBecker It's been a part of the standard since 1999. Some people don't know about it, others do but don't like it. Like you, I don't feel that strongly about it, but my practice is to always omit it. \$\endgroup\$Edward– Edward2015年07月13日 16:53:52 +00:00Commented Jul 13, 2015 at 16:53
-
\$\begingroup\$ This little bit of tautological madness will make use of the high bits of
rand()
s result to give a better distribution of values:(rand() % ((RAND_MAX / WIDTH)*WIDTH))/(RAND_MAX / WIDTH)
. It determines the largest possible multiple ofWIDTH
that will fit inside the range ofrand
and multiplies by that before taking the modules. This means the range of values used is much larger. Then a quantization step is applied to get the correct range of values. Example. \$\endgroup\$Emily L.– Emily L.2015年07月13日 18:07:31 +00:00Commented Jul 13, 2015 at 18:07 -
\$\begingroup\$ Or if you want to go the floating point route:
int(rand()/(std::nexttoward(RAND_MAX, std::numeric_limits<double>::infinity())*WIDTH)
. \$\endgroup\$Emily L.– Emily L.2015年07月13日 18:13:30 +00:00Commented Jul 13, 2015 at 18:13 -
\$\begingroup\$ @Edward Thanks a lot for your feedback! I will try to apply all of that. I have some questions though: How could I handle the input without the
getch()
andkbhit()
functions? How do I use#ifdef WINDOWS
to make the code compile on other platforms? Is it possible to do something likechar c = 'a';
for the Esc character or is it necessary to do something like#define ESC 27
char c = ESC;
? \$\endgroup\$Jannik– Jannik2015年07月13日 20:12:33 +00:00Commented Jul 13, 2015 at 20:12
Don't use raw pointers for your arrays. Instead use std::vector
, which is no less efficient but much less prone to errors.
Additionally, to implement a 2D array, you should use a single array and do arithmetic to find the index instead of using an array of arrays.
bool exit = false;
If you get rid of that line and change
while( !exit )
to
while ( true )
And then change
while( input != '1' && input != 27 ) {
to
while ( input != '1' )
{
if ( input == 27 )
{
return EXIT_SUCCESS;
}
You can get rid of the repetitive
if( input == 27) exit = true;
You could also say break
rather than return EXIT_SUCCESS
, but I find this more direct in this case.
I would also agree with changing 27
to a constant but that's outside the point that I was making.
You may also want to clear the screen before returning. Since I switched to the block form of the if
, that's as simple as adding a single line.