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

Snake game for Windows console

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
snakegameforwindowsconsole

Problem

I would apreciate any suggestions on how to make my code better and more efficient.

```
#include
#include
#include

struct SnakeNode
{
int x;
int y;
struct SnakeNode *next;
};

struct Food
{
int x;
int y;
int isEaten;
};

void Gotoxy(int column,int row);
int CreateScoreFile();
void CreateSnake(struct SnakeNode **snake);
void Graphics(struct SnakeNode *snake,struct Food food,int score,int highscore);
int isSnake(int x,int y,struct SnakeNode *snake);
void CreateFood(struct Food food,struct SnakeNode snake);
int GetSnakeSize(struct SnakeNode *snake);
struct SnakeNode GetListItem(struct SnakeNode snake,int index);
int lose(struct SnakeNode *snake);
void SaveScore(int score);
void Physics(struct SnakeNode **snake,int direction,struct Food food,int score,int highscore,int *endgame);
void DestroySnake(struct SnakeNode **snake);

char map[20][50] = {"##################################################",
"# #",
"# #",
"# #",
"# #",
"# #",
"# #",
"# #",
"# #",
"# #",
"# #",
"# #",
"# #",
"# #",
"# #",

Solution

Inefficient movement

I'm going to focus this review on one specific part of your code, which is there part where you move the snake. This part could be greatly improved. I'll show several areas where the code could be improved.

Don't use pointers everywhere

You currently pass in pointers such as direction and snake to your functions. But you don't actually change the value of direction or snake. So instead of passing pointers to these, just pass the values. That way, instead of direction and snake, you could just write direction and snake.

Extract common code

The big thing that stands out is that there is a lot of repetition in the code. There are four directions of movement and each direction uses the same huge chunk of code. Here is one direction:

else if(*direction == 3)
{
    for(i=GetSnakeSize(*snake)-1;i>0;i--)
    {
        lnode = GetListItem(*snake,i);

        fnode = GetListItem(*snake,i-1);

        lnode->x = fnode->x;
        lnode->y = fnode->y;
    }

    (*snake)->x = (*snake)->x - 1;

    if((*snake)->x x = 48;
    }
}


In each of the four directions, you do the part where you move the segments of the snake forward (except for the head). So you could extract all that common code out.

// This movement part was repeated 4 times before.  Now it's here once.
for(i=GetSnakeSize(snake)-1;i>0;i--)
{
    lnode = GetListItem(snake,i);

    fnode = GetListItem(snake,i-1);

    lnode->x = fnode->x;
    lnode->y = fnode->y;
}

// These four parts just move the head in various directions.
if (direction == 1)
{
    snake->x++;
    if (snake->x > 48) {
        snake->x = 1;
    }
}
else if (direction == 2)
{
    // etc.
}


\$O(n^2)\$ algorithm

Your snake segment movement is not very good. For each segment, it finds its node in the linked list, then finds the node in front of it, and then moves the segment. This ends up being \$O(n^2)\$, where \$n\$ is the length of the snake. You could do in \$O(n)\$ time by doing a one pass run through the snake like this:

if (snake != NULL)
{
    int prevX = snake->x;
    int prevY = snake->y;

    for (SnakeNode *seg = snake->next; seg != NULL; seg = seg->next) {
        int curX = seg->x;
        int curY = seg->y;
        seg->x = prevX;
        seg->y = prevY;
        prevX = curX;
        prevY = curY;
    }
}

Code Snippets

else if(*direction == 3)
{
    for(i=GetSnakeSize(*snake)-1;i>0;i--)
    {
        lnode = GetListItem(*snake,i);

        fnode = GetListItem(*snake,i-1);

        lnode->x = fnode->x;
        lnode->y = fnode->y;
    }

    (*snake)->x = (*snake)->x - 1;

    if((*snake)->x < 1)
    {
        (*snake)->x = 48;
    }
}
// This movement part was repeated 4 times before.  Now it's here once.
for(i=GetSnakeSize(snake)-1;i>0;i--)
{
    lnode = GetListItem(snake,i);

    fnode = GetListItem(snake,i-1);

    lnode->x = fnode->x;
    lnode->y = fnode->y;
}

// These four parts just move the head in various directions.
if (direction == 1)
{
    snake->x++;
    if (snake->x > 48) {
        snake->x = 1;
    }
}
else if (direction == 2)
{
    // etc.
}
if (snake != NULL)
{
    int prevX = snake->x;
    int prevY = snake->y;

    for (SnakeNode *seg = snake->next; seg != NULL; seg = seg->next) {
        int curX = seg->x;
        int curY = seg->y;
        seg->x = prevX;
        seg->y = prevY;
        prevX = curX;
        prevY = curY;
    }
}

Context

StackExchange Code Review Q#98285, answer score: 5

Revisions (0)

No revisions yet.