11
\$\begingroup\$

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

#include <stdio.h>
#include <time.h>
#include <windows.h>
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] = {"##################################################",
 "# #",
 "# #",
 "# #",
 "# #",
 "# #",
 "# #",
 "# #",
 "# #",
 "# #",
 "# #",
 "# #",
 "# #",
 "# #",
 "# #",
 "# #",
 "# #",
 "# #",
 "# #",
 "##################################################"};
int main()
{
 int endgame = 0;
 int gamespeed = 25;
 int score = 0;
 int highscore = 0;
 int direction = 1;
 struct SnakeNode *snake = NULL;
 struct Food food;
 food.isEaten = 1;
 srand(time(NULL));
 highscore = CreateScoreFile();
 CreateSnake(&snake);
 do
 {
 CreateFood(&food,snake);
 Graphics(snake,food,score,highscore);
 Sleep(gamespeed);
 system("CLS");
 Physics(&snake,&direction,&food,&score,&highscore,&endgame);
 }while(endgame == 0);
 DestroySnake(&snake);
 return 0;
}
void Gotoxy(int column,int row)
{
 COORD c;
 c.X = column;
 c.Y = row;
 SetConsoleCursorPosition(GetStdHandle(STD_OUTPUT_HANDLE),c);
}
int CreateScoreFile()
{
 FILE *scfile;
 int highscore = 0;
 scfile = fopen("Score file.txt","r");
 if(scfile == NULL)
 {
 scfile = fopen("Score file.txt","w");
 fprintf(scfile,"%d",highscore);
 }
 else
 {
 fscanf(scfile,"%d",&highscore);
 }
 fclose(scfile);
 return highscore;
}
void CreateSnake(struct SnakeNode **snake)
{
 struct SnakeNode *hnext = NULL;
 struct SnakeNode *hprev = NULL;
 int x = 27;
 int y = 10;
 int size = 0;
 *snake = (struct SnakeNode *)malloc(sizeof(struct SnakeNode));
 (*snake)->x = x;
 (*snake)->y = y;
 (*snake)->next = NULL;
 hprev = *snake;
 hnext = (*snake)->next;
 while(hnext != NULL || size < 4)
 {
 x--;
 hnext = (struct SnakeNode *)malloc(sizeof(struct SnakeNode));
 hnext->x = x;
 hnext->y = y;
 hnext->next = NULL;
 hprev->next = hnext;
 hprev = hnext;
 hnext = hnext->next;
 size++;
 }
}
void Graphics(struct SnakeNode *snake,struct Food food,int score,int highscore)
{
 int x,y;
 struct SnakeNode *temp = NULL;
 for(y=0;y<20;y++)
 {
 for(x=0;x<50;x++)
 {
 printf("%c",map[y][x]);
 }
 printf("\n");
 }
 Gotoxy(food.x,food.y);
 printf("$");
 temp = snake;
 while(temp != NULL)
 {
 Gotoxy(temp->x,temp->y);
 printf("*");
 temp = temp->next;
 }
 Gotoxy(0,20);
 printf("\nScore: %d",score);
 if(score <= highscore)
 {
 printf(" ------ Highscore: %d",highscore);
 }
 else
 {
 printf(" ------ Highscore: %d",score);
 }
}
int isSnake(int x,int y,struct SnakeNode *snake)
{
 struct SnakeNode *temp = NULL;
 temp = snake;
 while(temp != NULL)
 {
 if(temp->x == x && temp->y == y)
 {
 return 1;
 }
 temp = temp->next;
 }
 return 0;
}
void CreateFood(struct Food *food,struct SnakeNode *snake)
{
 if(food->isEaten)
 {
 food->x = rand()%48+1;
 food->y = rand()%18+1;
 food->isEaten = 0;
 do
 {
 if(isSnake(food->x,food->y,snake))
 {
 food->x = rand()%48+1;
 food->y = rand()%18+1;
 }
 }while(isSnake(food->x,food->y,snake));
 }
}
struct SnakeNode * GetListItem(struct SnakeNode *snake,int index)
{
 int i;
 struct SnakeNode *node = NULL;
 node = snake;
 for(i=0;i<index;i++)
 {
 node = node->next;
 }
 return node;
}
int GetSnakeSize(struct SnakeNode *snake)
{
 int size = 0;
 while(snake != NULL)
 {
 size++;
 snake = snake->next;
 }
 return size;
}
int lose(struct SnakeNode *snake)
{
 struct SnakeNode *iterator;
 iterator = snake->next;
 while(iterator != NULL)
 {
 if(snake->x == iterator->x && snake->y == iterator->y)
 {
 return 1;
 }
 iterator = iterator->next;
 }
 return 0;
}
void SaveScore(int score)
{
 FILE *savscore;
 int filescore;
 savscore = fopen("Score file.txt","r");
 if(savscore != NULL)
 {
 fscanf(savscore,"%d",&filescore);
 if(score > filescore)
 {
 fclose(savscore);
 savscore = fopen("Score file.txt","w");
 fprintf(savscore,"%d",score);
 }
 }
 fclose(savscore);
}
void Physics(struct SnakeNode **snake,int *direction,struct Food *food,int *score,int *highscore,int *endgame)
{
 int i;
 struct SnakeNode *lnode = NULL;
 struct SnakeNode *fnode = NULL;
 struct SnakeNode *nnode = NULL;
 if(GetAsyncKeyState(VK_RIGHT))
 {
 if(*direction != 3)
 {
 *direction = 1;
 }
 }
 else if(GetAsyncKeyState(VK_LEFT))
 {
 if(*direction != 1)
 {
 *direction = 3;
 }
 }
 else if(GetAsyncKeyState(VK_UP))
 {
 if(*direction != 2)
 {
 *direction = 4;
 }
 }
 else if(GetAsyncKeyState(VK_DOWN))
 {
 if(*direction != 4)
 {
 *direction = 2;
 }
 }
 if(*direction == 1)
 {
 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 > 48)
 {
 (*snake)->x = 1;
 }
 }
 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;
 }
 }
 else if(*direction == 4)
 {
 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)->y = (*snake)->y - 1;
 if((*snake)->y < 1)
 {
 (*snake)->y = 18;
 }
 }
 else
 {
 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)->y = (*snake)->y + 1;
 if((*snake)->y > 18)
 {
 (*snake)->y = 1;
 }
 }
 if(lose(*snake))
 {
 *endgame = 1;
 Graphics(*snake,*food,*score,*highscore);
 SaveScore(*score);
 Beep(250,250);
 Sleep(2000);
 }
 if((*snake)->x == (*food).x && (*snake)->y == (*food).y)
 {
 (*food).isEaten = 1;
 *score = *score + 10;
 Beep(1000,25);
 lnode = GetListItem(*snake,GetSnakeSize(*snake)-1);
 nnode = (struct SnakeNode *)malloc(sizeof(struct SnakeNode));
 nnode->x = lnode->x;
 nnode->y = lnode->y;
 nnode->next = NULL;
 lnode->next = nnode;
 }
}
void DestroySnake(struct SnakeNode **snake)
{
 struct SnakeNode *temp = NULL;
 while(*snake != NULL)
 {
 temp = *snake;
 *snake = (*snake)->next;
 free(temp);
 }
}
200_success
145k22 gold badges190 silver badges478 bronze badges
asked Jul 28, 2015 at 1:24
\$\endgroup\$
1
  • 2
    \$\begingroup\$ You should specify what you need help on in the question. It helps the reviewers understand what's going on so they know what focus on. This might help: meta.codereview.stackexchange.com/questions/2436/… \$\endgroup\$ Commented Jul 28, 2015 at 1:43

1 Answer 1

5
\$\begingroup\$

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 < 1)
 {
 (*snake)->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;
 }
}
answered Jul 29, 2015 at 5:59
\$\endgroup\$
0

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.