patterncMinor
Snake game for Windows console
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] = {"##################################################",
"# #",
"# #",
"# #",
"# #",
"# #",
"# #",
"# #",
"# #",
"# #",
"# #",
"# #",
"# #",
"# #",
"# #",
```
#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
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:
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.
\$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:
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.