patterncModerate
Simple Word Search Game
Viewed 0 times
wordgamesimplesearch
Problem
I've created a fairly simple word search generator/solver. I'm looking to improve on picking the right algorithm to tackle problems like this, so any criticisms on my code would be greatly appreciated :)
```
#include
#include
#include
#include
#include //for stop watch
#include //for failure sound
struct point{ //represents a point in the grid
int x;
int y;
};
char grid[100][100]; //the answer grid
int color[100][100];
int ctr=0;
int grid_size; //size of the grid is grid_size*grid_size
char nullchar='z'; //the NULL character chosen for the grid because the grid will never have lowercase alphabets so any lowercase alphabet can be used as NULL character
int max_words; //number of words that are inserted in the grid
enum direction{ //enumerator for direction in the grid in which the word is inserted
UP=1, //start the count from 1
DOWN,
LEFT,
RIGHT,
UP_LEFT,
UP_RIGHT,
DOWN_LEFT,
DOWN_RIGHT
};
/data to choose the words from/
//
char *animals[]={"deer","sheep","dog","cat","lion","tiger","elephant","wolf","hen"};
int animals_size=9;
char *fruits[]={"banana","apple","pear","grapes","guava","papaya","tomato","mango","lychee","pineapple","cherry"};
int fruits_size=11;
char *games[]={"cricket","baseball","football","tennis","rugby","boxing","wrestling","squash","bowling"};
int games_size=9;
char *starwars[]={"boba","anakin","yoda","maul","palpatine","vader","lightsaber","hansolo","millenium","llando"};
int starwars_size=10;
char *input[100]; //the words that are inserted in the grid
int input_point[100][3];//the x,y coordinate and direction of the words that are inserted in the grid
int mark[100]={0};//used to mark which all words have been identified by the user so that there is no duplicate answer accepted
int flag=0; //flag
struct point shift_point(struct point start, enum direction d){ //start is the position of the point to be shifted in the grid, d is the direction in which the point has to shift
in
```
#include
#include
#include
#include
#include //for stop watch
#include //for failure sound
struct point{ //represents a point in the grid
int x;
int y;
};
char grid[100][100]; //the answer grid
int color[100][100];
int ctr=0;
int grid_size; //size of the grid is grid_size*grid_size
char nullchar='z'; //the NULL character chosen for the grid because the grid will never have lowercase alphabets so any lowercase alphabet can be used as NULL character
int max_words; //number of words that are inserted in the grid
enum direction{ //enumerator for direction in the grid in which the word is inserted
UP=1, //start the count from 1
DOWN,
LEFT,
RIGHT,
UP_LEFT,
UP_RIGHT,
DOWN_LEFT,
DOWN_RIGHT
};
/data to choose the words from/
//
char *animals[]={"deer","sheep","dog","cat","lion","tiger","elephant","wolf","hen"};
int animals_size=9;
char *fruits[]={"banana","apple","pear","grapes","guava","papaya","tomato","mango","lychee","pineapple","cherry"};
int fruits_size=11;
char *games[]={"cricket","baseball","football","tennis","rugby","boxing","wrestling","squash","bowling"};
int games_size=9;
char *starwars[]={"boba","anakin","yoda","maul","palpatine","vader","lightsaber","hansolo","millenium","llando"};
int starwars_size=10;
char *input[100]; //the words that are inserted in the grid
int input_point[100][3];//the x,y coordinate and direction of the words that are inserted in the grid
int mark[100]={0};//used to mark which all words have been identified by the user so that there is no duplicate answer accepted
int flag=0; //flag
struct point shift_point(struct point start, enum direction d){ //start is the position of the point to be shifted in the grid, d is the direction in which the point has to shift
in
Solution
Adding to what other reviews have already mentioned:
-
Try to avoid global data. However, if you do use them, the least harmful kind of globals would be the file scoped ones. How to make a variable or function file scoped in C? Prefix the variable type with the
Now those variables cannot be hijacked by other source files in your project by means of
-
Reiterating on the
-
You could
Now you can reference it simply as
-
You should pick a more consistent naming style and stick to it. Right now, your code has a mix of
-
In
Hoisting that call is an easy optimization for the compiler to do, but still, it looks silly in the code. You should instead store that value inside a local variable. While you are at it, be consistent with the types and use
-
Avoid noisy and obvious comment, like "loop variable" and "function to generate a random char" by the side of a
-
I suggest better spacing your code. A space between arithmetical operators and operands is generally good and facilitates reading, for the same reasons that we use spaces when writing plain text. For example:
Versus:
-
If your code is targeting C99 and above (which you should definitely for new code; older standards are only for maintaining legacy projects), then you can and should declare variables as close to their first use as it is possible, with the intent of reducing scope. Variables declared at the top of a function are like mini-globals. C99 also allows you to declare
-
A few variables in your code have very vague names, such as
-
Try to avoid global data. However, if you do use them, the least harmful kind of globals would be the file scoped ones. How to make a variable or function file scoped in C? Prefix the variable type with the
static qualifier when declaring it. E.g.:static char grid[100][100];
static int color[100][100];
static int ctr = 0;Now those variables cannot be hijacked by other source files in your project by means of
extern. This is good since now the programmer has a hint that those variables are not changed outside the current source file. This can save someone from a lot of stress when trying to debug complex code in multi source file projects.-
Reiterating on the
constant point: Mark as much things as you can with const. The great majority of software bugs stem from the programmer not knowing in which state his/her program is. By making variables and pointers const, you have a compiler enforced guarantee that the data won't be changed inside a given context, thus anyone should be able to track down its current state by simple backtracking til the declaration. Apply const to your immutable string arrays, like animals and fruits, but also to function parameters that take read-only pointers to external data, like in:int check_insert(const char* word, struct point start, enum direction d)
^^^^^word is not supposed to be changed by the function. So make that a hard constraint.-
You could
typedef the point structure and the direction enum to make usage more succinct, without having to add the struct/enum tags all the time:typedef struct {
int x;
int y;
} point;Now you can reference it simply as
point. Same could be done to direction.-
You should pick a more consistent naming style and stick to it. Right now, your code has a mix of
snake_case and camelCase, which make it harder to read and remember. Pick one notation and use it consistently through the code. You might also consider using something like PascalCase to differentiate User Defined Types from native types and function. E.g. Point and Direction instead.-
In
check_insert(), you should avoid calling strlen() inside the condition check of the while loop:while(i < (int)strlen(word))Hoisting that call is an easy optimization for the compiler to do, but still, it looks silly in the code. You should instead store that value inside a local variable. While you are at it, be consistent with the types and use
size_t for everything, which is the type returned by strlen(), removing the type cast:size_t i = 0;
size_t wordLen = strlen(word);
while (i < wordLen) {
...-
Avoid noisy and obvious comment, like "loop variable" and "function to generate a random char" by the side of a
generate_random_char(). This is pure visual pollution. Use comments to explain why you did something. Comments explaining what the code does are an indicative of poorly written code, since the reader should be able to figure out what's being done just by reading the code itself, if it is clear enough.-
I suggest better spacing your code. A space between arithmetical operators and operands is generally good and facilitates reading, for the same reasons that we use spaces when writing plain text. For example:
for(i=0;i<grid_size;i++){Versus:
for (i = 0; i < grid_size; i++) {-
If your code is targeting C99 and above (which you should definitely for new code; older standards are only for maintaining legacy projects), then you can and should declare variables as close to their first use as it is possible, with the intent of reducing scope. Variables declared at the top of a function are like mini-globals. C99 also allows you to declare
for loop counters directly inside the loop. E.g.:for (int i = 0; i < grid_size; i++) {
for (int j = 0; j < grid_size; j++) {-
A few variables in your code have very vague names, such as
flag, arr and mark. Try to be more descriptive of their purpose.Code Snippets
static char grid[100][100];
static int color[100][100];
static int ctr = 0;int check_insert(const char* word, struct point start, enum direction d)
^^^^^typedef struct {
int x;
int y;
} point;while(i < (int)strlen(word))size_t i = 0;
size_t wordLen = strlen(word);
while (i < wordLen) {
...Context
StackExchange Code Review Q#88733, answer score: 10
Revisions (0)
No revisions yet.