patterncppMinor
ASCII-based snake game in C++
Viewed 0 times
gameasciisnakebased
Problem
I have recently finished developing my ASCII based snake game. As a newbie to C++ I would appreciate it if I was given feedback as to how I could improve my code when it comes to efficiency and readability.
```
#include
#include
#include
#include
using std::cout; using std::cin; using std::endl;
const int width = 20;
const int height = 20;
bool gameState = true;
int score;
struct fruitClass {
int x;
int y;
};
struct Node {
int x;
int y;
struct Node *next;
};
Node *newNode(int x, int y) {
Node *link = new(Node);
link->x = x;
link->y = y;
return link;
}
void rotateList(Node *head, int x, int y) {
int i;
Node *current = head;
int tempX[2], tempY[2];
for (i = 0; current != nullptr; i++) {
if (i == 0) {
tempX[0] = current->x;
tempY[0] = current->y;
current->x = x;
current->y = y;
}
else {
tempX[(i % 2 == 0) ? 0 : 1] = current->x;
tempY[(i % 2 == 0) ? 0 : 1] = current->y;
current->x = tempX[(i % 2 == 0) ? 1 : 0];
current->y = tempY[(i % 2 == 0) ? 1 : 0];
}
current = current->next;
}
}
void append(Node &head, Node &link) {
Node *ptr;
ptr = head;
head = link;
head->next = ptr;
}
Node *head;
fruitClass fruit;
class snakeClass {
private:
int posX;
int posY;
int tailLen = 0;
enum eDirection{STOP = 0, UP, LEFT, RIGHT, DOWN};
eDirection dir;
public:
void init () {
//Initialise snake's position
posX = width/2;
posY = height/2;
//Initialise first tail node
head = newNode(posX, posY);
//Spawn fruit
fruit.x = rand() % width;
fruit.y = rand() % height;
//Initialise score
score = 0;
//Initialise direction
dir = STOP;
}
void draw() {
system("cls");
//Draw top of playing field
for (int i = 0; i y && j == curr
```
#include
#include
#include
#include
using std::cout; using std::cin; using std::endl;
const int width = 20;
const int height = 20;
bool gameState = true;
int score;
struct fruitClass {
int x;
int y;
};
struct Node {
int x;
int y;
struct Node *next;
};
Node *newNode(int x, int y) {
Node *link = new(Node);
link->x = x;
link->y = y;
return link;
}
void rotateList(Node *head, int x, int y) {
int i;
Node *current = head;
int tempX[2], tempY[2];
for (i = 0; current != nullptr; i++) {
if (i == 0) {
tempX[0] = current->x;
tempY[0] = current->y;
current->x = x;
current->y = y;
}
else {
tempX[(i % 2 == 0) ? 0 : 1] = current->x;
tempY[(i % 2 == 0) ? 0 : 1] = current->y;
current->x = tempX[(i % 2 == 0) ? 1 : 0];
current->y = tempY[(i % 2 == 0) ? 1 : 0];
}
current = current->next;
}
}
void append(Node &head, Node &link) {
Node *ptr;
ptr = head;
head = link;
head->next = ptr;
}
Node *head;
fruitClass fruit;
class snakeClass {
private:
int posX;
int posY;
int tailLen = 0;
enum eDirection{STOP = 0, UP, LEFT, RIGHT, DOWN};
eDirection dir;
public:
void init () {
//Initialise snake's position
posX = width/2;
posY = height/2;
//Initialise first tail node
head = newNode(posX, posY);
//Spawn fruit
fruit.x = rand() % width;
fruit.y = rand() % height;
//Initialise score
score = 0;
//Initialise direction
dir = STOP;
}
void draw() {
system("cls");
//Draw top of playing field
for (int i = 0; i y && j == curr
Solution
Oh boy! A snake game. I'm excited, since this was also one of my first games. So let's get to work, shall we?
Keep it short and simple
The biggest concern with your code gets obvious if we just have a look at your post. Your whole code is inside a single file, which makes it hard to navigate. Your file ist "just" 270 lines though, but still, it's a first step to make your code more readable.
Consistency is key
The next concern is missing consistency. You provide a class for your snake, but a
Also, you're calling your struct
This leads to
Make your functions easy to use, but hard to use wrong
This one is hard to explain, so let's start with
There are several things going on here that can go wrong. For one, you forgot to set
If
However, at this point we're just constructing a value, which can be done with
This makes it tremendously harder to use
Oh, and you use
Rotating is easy with the correct data structure
You're
Still, let's try it instead. First, instead of having
Now, instead of a global
I'm not kidding. That's the whole logic behind your
Either way, now that you have a
Be aware of global state
I get it. It's just Snake. So there's no harm in using global variables, right?
Wrong. Here's a little experiment: let's say you want to save and load all necessary variables for your game, so that a player can save it to disk, and continue later. Can you identify all those variables without scrolling in your code?
Is it likely that you forget one of those variables? Remember, make your functions easy to use, but hard to use wrong. For your game, I recommend you to create a small class
It doesn't need to be a class, it can also be a small struct, but it's usually a good idea to have something that handles the last few bytes. Your
That way, you can easily ask the user for a new game:
You can then even provide keys to save/load your game, which helps tremendously with debugging.
Keep it short and simple
We've had this heading already, but it's here for a secon
Keep it short and simple
The biggest concern with your code gets obvious if we just have a look at your post. Your whole code is inside a single file, which makes it hard to navigate. Your file ist "just" 270 lines though, but still, it's a first step to make your code more readable.
Consistency is key
The next concern is missing consistency. You provide a class for your snake, but a
struct with C-like object oriented programming Node. That's somewhat arbitrary.Also, you're calling your struct
Node, which makes it easy to disambiguate it from normal variables (which use camelCase), whereas your other class snakeClass starts with a lowercase, and the Class suffix is superflous. Instead, pick a naming convention that immediately shows whether you have a class-like type, a function or a simple variable.This leads to
struct Node;
struct Fruit;
struct Snake;Make your functions easy to use, but hard to use wrong
This one is hard to explain, so let's start with
newNode:Node *newNode(int x, int y) {
Node *link = new(Node);
link->x = x;
link->y = y;
return link;
}There are several things going on here that can go wrong. For one, you forgot to set
link->next. For another, you use new and Node*, which are both not necessary and can lead to memory leaks if you accidentally use newNode:void foo(){
ptr = newNode(x,y);
if(some_condition) {
addPtr(ptr);
} else {
return;
}
}If
some_condition was wrong, we've just leaked memory. Instead, have newNode return a value:Node newNode(int x, int y) {
Node node;
node.x = x;
node.y = y;
node.next = nullptr;
return node;
}However, at this point we're just constructing a value, which can be done with
{} or a constructor:Node node = {x, y, nullptr};
// or define a constructor:
struct Node {
Node(int x, int y, Node * next = nullptr) : x(x),y(y),next(next){}
int x;
int y;
Node * next;
}This makes it tremendously harder to use
newNode in a wrong way.Oh, and you use
newHead wrong. Remember snakeClass::init()? If you ever want to play multiple games in a row, you're going to leak memory. More on that later, see "global state" below.Rotating is easy with the correct data structure
You're
Node is essentially a single-linked list, which makes rotating awkward. You can, however, rotate in O(1) if you use the correct data structure: a double-linked list. However, it's not very memory friendly, since you would have to create a new node in every step of your snake.Still, let's try it instead. First, instead of having
next in Node, let's create a simpler variant, SnakePart:struct SnakePart {
int x;
int y;
};Now, instead of a global
head, let's add a function in your snakeClass, called advance, and a std::deque:class SnakePart {
std::deque parts;
void advance(int x, int y, bool loose_tail = true) {
parts.emplace_front({x,y});
if(loose_tail)
parts.pop_back();
}
};I'm not kidding. That's the whole logic behind your
rotateList, written in a double-ended queue. Note that this is still not optimal, since you will usually only change a single element. I guess that a std::vector with an additional index will be more cache friendly, but that's up to benchmarking. Either way, now that you have a
std container, you can use all algorithms, iterators and other nice things.Be aware of global state
I get it. It's just Snake. So there's no harm in using global variables, right?
Wrong. Here's a little experiment: let's say you want to save and load all necessary variables for your game, so that a player can save it to disk, and continue later. Can you identify all those variables without scrolling in your code?
Is it likely that you forget one of those variables? Remember, make your functions easy to use, but hard to use wrong. For your game, I recommend you to create a small class
Game that handles all this data for you:class Game {
Snake snake;
Fruit fruit;
Score score;
Time time;
public:
...
};It doesn't need to be a class, it can also be a small struct, but it's usually a good idea to have something that handles the last few bytes. Your
snakeClass almost provides this functionality, although several variables are still global instead of local.That way, you can easily ask the user for a new game:
int main(){
char answer = 'y';
do {
playGame();
std::cout << "Do you want to play again? y/n: ";
answer = getAnswer();
} while(answer != 'n');
}You can then even provide keys to save/load your game, which helps tremendously with debugging.
Keep it short and simple
We've had this heading already, but it's here for a secon
Code Snippets
struct Node;
struct Fruit;
struct Snake;Node *newNode(int x, int y) {
Node *link = new(Node);
link->x = x;
link->y = y;
return link;
}void foo(){
ptr = newNode(x,y);
if(some_condition) {
addPtr(ptr);
} else {
return;
}
}Node newNode(int x, int y) {
Node node;
node.x = x;
node.y = y;
node.next = nullptr;
return node;
}Node node = {x, y, nullptr};
// or define a constructor:
struct Node {
Node(int x, int y, Node * next = nullptr) : x(x),y(y),next(next){}
int x;
int y;
Node * next;
}Context
StackExchange Code Review Q#142926, answer score: 7
Revisions (0)
No revisions yet.