HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

ASCII-based snake game in C++

Submitted by: @import:stackexchange-codereview··
0
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

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 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.