6
\$\begingroup\$

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<stdio.h>
#include<string.h>
#include<stdlib.h>
#include<ctype.h>
#include<time.h>  //for stop watch
#include<windows.h>  //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
    int i = start.x;
    int j = start.y;
    struct point new_point;  //final position of the shifted point depends on the direction d
    switch(d){
    //eight directions in which the point can move
        case UP:
            new_point.x = i-1; //Move up a row
            new_point.y = j;
            break;
        case DOWN:
            new_point.x = i+1;  //Move down a row
            new_point.y = j;
            break;
        case LEFT:
            new_point.x = i;
            new_point.y = j-1; //Column moves left
            break;
        case RIGHT:
            new_point.x = i;
            new_point.y = j+1; //Column moves right
            break;
        case UP_LEFT:
            new_point.x = i-1; //Row moves up
            new_point.y = j-1; //Column moves left
            break;
        case UP_RIGHT:
            new_point.x = i-1; //Row moves up
            new_point.y = j+1; //Column moves right
            break;
        case DOWN_LEFT:
            new_point.x = i+1; //Row moves down
            new_point.y = j-1; //Column moves to left
            break;
        case DOWN_RIGHT:
            new_point.x = i+1; //Row moves down
            new_point.y = j+1; //Column moves right
            break;
        default:
            new_point.x = i; //Row stays the same
            new_point.y = j; //Column stays the same
            break;
    }
//Handle out of bounds errors
    if(new_point.x < -1 || new_point.x > grid_size || new_point.y < -1 || new_point.y > grid_size){
        flag=1;   //exception handling
    }
    return new_point;  //return the new point
}


int check_insert(char* word, struct point start,enum direction d){  //function to check whether a word can be inserted in the grid or not at that position(start)
    int i = 0;   //loop variable
    struct point new_point = start;
    while(i < (int)strlen(word)) //Iterates through the word char array
    {
    //Attempt to shift the point to the new point
        if(grid[new_point.x][new_point.y] == nullchar||grid[new_point.x][new_point.y]==word[i]){
            new_point = shift_point(new_point,d);
            i++;
        }
        else{
            return 0;   //if cant be inserted return false
        }

        if(flag==1)
            return 0;  //this flag has been set in shift_point function due to array bounds exception
    }
    return 1;  //can be inserted so return true
}

int arr[10]={1,3,2,4,5,6,1,3,2,4};  //this array ensures 40,40,20 percent division of words to horizontal, vertical and diagonal direction
int p=0;   //used as an index to traverse the above array arr

void insertWordInGrid(char *word,int i)   //function to insert word in the grid
{  //i signifies that the i th word is being inserted

    struct point place; //point where the word is to inserted in the grid
    enum direction d;
    do{
        place.x = rand() % grid_size; //set to a random row
        place.y = rand() % grid_size; //set to a random column
        d = (arr[(p++)%10]); //get a direction according to the rule specified

    }
    while(!check_insert(word,place,d));  //run the loop until we cant insert the word

    int j = 0;//loop variable

    struct point new_point = place;
    while(j < (int)strlen(word)){
        grid[new_point.x][new_point.y] = (char)toupper(word[j]);   //insertion into the grid
        new_point = shift_point(new_point,d);   //shift according to direction
        j++;
    }
    input_point[i][0]=place.x;  //used in changing the grid afterwards when user enters the correct answer
    input_point[i][1]=place.y;
    input_point[i][2]=d;

}

char generate_random_char(){   //function to generate a random char
    return 'A' + rand()%26;
}

void fill_grid(){  //fill the remaining places with random characters
    int i,k;
    for(i=0;i<grid_size;i++){
        for(k=0;k<grid_size;k++){
            if(grid[i][k] == nullchar){
                grid[i][k] = generate_random_char(); //Set every null value to a random character
            }
        }
    }
}


void printGrid()   //function to print the grid
{
    int i,j;
    for(i=0;i<grid_size;i++){
        for(j=0;j<grid_size;j++){
            HANDLE h = GetStdHandle ( STD_OUTPUT_HANDLE );
            WORD wOldColorAttrs;
            CONSOLE_SCREEN_BUFFER_INFO csbiInfo;

  /*
   * First save the current color information
   */
            GetConsoleScreenBufferInfo(h, &csbiInfo);
            wOldColorAttrs = csbiInfo.wAttributes;

  /*
   * Set the new color information
   */
            SetConsoleTextAttribute ( h, color[i][j] );



  /*
   * Restore the original colors
   */


            printf("%c ",grid[i][j]);
            SetConsoleTextAttribute ( h, wOldColorAttrs);
        }
        printf("\n");
    }
}

int checkans(char *str){  //function to check whether str is in the grid or not
    ctr++;//color counter
    int i=0;

    int flag=0;
    for(i=0;i<max_words;i++){  //check in input array 

        if(strcmp(str,input[i])==0){  //condition satisfied and str is in the grid
            if (mark[i]==1){
                return 0;
            }


            flag=1;
            break;
        }

    }
    if(flag==1){  //then str is in the grid and change the grid
        mark[i]=1;
        int x=0;
        struct point p;
        p.x=input_point[i][0];
        p.y=input_point[i][1];
        enum direction d=input_point[i][2];
        for(x=0;x<strlen(str);x++){
            color[p.x][p.y]=ctr
            //grid[p.x][p.y]='*';    //change the grid where the word is there
            p=shift_point(p,d);
        }
        return 1;   //true condition
    }
    else
        return 0;   //false condition
}


int main(){
    srand(1);   //seed the random number generator
    printf("Welcome to Word Search 3000!\n\n\n");
    char name[100];
    int ans;
    printf("What's your name? ");
    scanf(" %s",&name);
    printf("\nHi %s!!! What category do you want to choose? ",name);
    printf("\nPress 1 for Animals..");
    printf("\nPress 2 for Fruits..");
    printf("\nPress 3 for Games..");
    printf("\nPress 4 for Star Wars..");

    scanf("%d",&ans);
    printf("\nEnter the size of the grid..");
    scanf("%d",&grid_size);
    int i,j;

    for(i=0;i<grid_size;i++){   //initialize the grid to null char 'z'   
        for(j=0;j<grid_size;j++){
            grid[i][j]='z';
            color[i][j]=15;
        }

    }

    max_words=grid_size-4;   //maximum words in the grid can be grid_size-4

    char *item;

    if(ans==1){
        item="animals";
        int i=0;
        if(max_words>animals_size)
            max_words=animals_size;
        for(i=0;i<max_words;i++){

            input[i]=animals[i];
            insertWordInGrid(animals[i],i);
        }

    }
    else if(ans==2){
        item="fruits";
        int i=0;
        if(max_words>fruits_size)
            max_words=fruits_size;
        for(i=0;i<max_words;i++){

            input[i]=fruits[i];
            insertWordInGrid(fruits[i],i);
        }

    }
    else if(ans==3){
        item="games";
        int i=0;
        if(max_words>games_size)
            max_words=games_size;
        for(i=0;i<max_words;i++){

            input[i]=games[i];
            insertWordInGrid(games[i],i);
        }
    }
    else if(ans==4){
        item="Star Wars";
        int i=0;
        if(max_words>starwars_size)
            max_words=starwars_size;
        for(i=0;i<max_words;i++){

            input[i]=starwars[i];
            insertWordInGrid(starwars[i],i);
        }
    }


    int t=max_words/4;   //time in minutes in which the user can do the puzzle
    system("cls");
    printf("Ok %s! There are %d %s hidden in this grid. Let's see if you can find them.\nYou have %d minutes to solve. GOOD LUCK!! \n\n",name,max_words,item,t);
    fill_grid();
    printGrid();

    clock_t start = clock();


    for(i=0;i<max_words;i++){
        char ans[50];

        clock_t end = clock();
        float seconds = (float)(end - start) / CLOCKS_PER_SEC;

        printf("\nYou have given %d right answers..",i);
        printf("\nTime Remaining: %f seconds",t*60-seconds);
        printf("\nEnter answer(in lower case)..");


        scanf(" %s",&ans);


        if(seconds>t*60){
            system("cls");
            printf("TIME UP!!!You suck!!!");
            return 1;
        }

        if(checkans(ans)){
            system("cls");
            printf("Your answer was correct!!\n");

            printGrid();
        }
        else{
            system("cls");
            printf("Wrong answer!!\n");
            Beep(1000,1000);
            printGrid();
            i--;
        }


    }
    system("cls");
    printGrid();
    printf("\nCongratulations!!!You win!!!");
    return 0;


}
\$\endgroup\$

4 Answers 4

10
\$\begingroup\$

Adding to what other reviews have already mentioned:

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

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

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

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

  5. 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) {
    ...
    
  6. 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.

  7. 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++) {
    
  8. 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++) {
    
  9. A few variables in your code have very vague names, such as flag, arr and mark. Try to be more descriptive of their purpose.

\$\endgroup\$
1
  • \$\begingroup\$ +1! I know typedefing this way is popular. I stopped doing it after reading and adopting most of the Linux kernel coding standard, and now I really appreciate knowing at first sight that a variable isn't a native type. \$\endgroup\$
    – Gauthier
    May 5, 2015 at 12:48
8
\$\begingroup\$

I haven't looked at everything, but I'll address many of the issues I've found at the surface.

  • It's nicer to have a space between the #include and the header:

    #include <stdio.h>
    
  • Be aware that with <windows.h> and system("cls"), this program is non-portable to other operating systems. If you don't want it this way, then you should consider having alternatives that work with other operating systems.

  • You have a lot of global variables, which should be avoided as much as possible. If a variable does need to be visible in this scope, then make them const, preventing such variables from being modified elsewhere.

    For everything else, try to keep them in the lowest scope possible by passing them to other functions as needed.

    For example, the arrays under the comment /*data to choose the words from*/ could all be const since it appears that they are only to be read.

  • Try to avoid having lengthy comments in a single line. Forcing the reader to scroll across the screen to read comments can be pretty annoying. Instead, place them in blocks above the individual line of code.

    You have some useless comments coupled with ambiguous names:

    char grid[100][100];  //the answer grid
    

    Just rename this to answer_grid and this comment would no longer be needed.

    You also have plain useless comments:

    int flag=0; //flag
    

    Of course it's a flag, but what is this particular flag for? Either put that in the comment or rename the variable as such.

    Many of the comments preceding the functions are also obvious. If the function's intent is already clear based on the name, then don't add a comment saying the same thing.

  • This doesn't look correct:

    srand(1);
    

    To proper seed the RNG, you'll need to use time() as such:

    srand(time(NULL));
    

    Other than that, I commend you for putting this call in the proper place. Many people I've seen tend to call it repeatedly in other functions, which causes the seed to be reset each time.

\$\endgroup\$
7
\$\begingroup\$

I'm new to this StackExchange so I'm not sure if this is entirely relevant but there's a neat trick you can use to vastly minimise the code you use to deal with directions.

By creating an array int dx[] = {0, 0, -1, 1} and int dy[] = {-1, 1, 0, 0} we can represent UP DOWN LEFT RIGHT in that order.

Suppose you had this:

enum direction {
    UP,
    DOWN,
    LEFT,
    RIGHT
};

Then you can simply write new_point.x = i + dx[d] and new_point.y = j + dy[d]

I'm sure you can see how to easily expand it to include the diagonal directions. The added benefit of doing it this way is that you can easily loop through the directions as well.

However, using an enum like this may be considered bad style - we shouldn't depend on the enums having integer values (even though they do). Maybe #define would work better with this method.

\$\endgroup\$
4
\$\begingroup\$

Infinite loop

I just want to make one quick comment. This code here that tries to randomly place the next word:

    do {
        place.x = rand() % grid_size; //set to a random row
        place.y = rand() % grid_size; //set to a random column
        d = (arr[(p++)%10]); //get a direction according to the rule specified    
    } while(!check_insert(word,place,d));  //run the loop until we cant insert the word

runs the risk of running forever in an infinite loop. If you make the grid too small or you have too many words, it might be impossible to fit the next word into the grid. One simple way of fixing this would be to keep track of how many tries you have attempted and have a MAX_TRIES constant that limits your tries. If you exceed this limit, you can either skip the word or exit with an error message.

\$\endgroup\$

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.