6
\$\begingroup\$

I'd like to know how this program can be improved. Any comments or critiques are most welcome.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <time.h>
#include <errno.h>
#define MAX 10000
#define FOUND 1
#define NOT_FOUND 0
#define DEAD 0
#define LINUX

void starMaker(char string[] , char starContainer[]); /*creats a string of stars */
int wordCalculator();/*calculates how many words there is in the dictionary file*/
int queryFinder(char letter , char string[] , char starWord[]);/*checks if the character entered is in the mystery word*/
int starFinder(char starWord[]);/*checks if there is anymore stars in the star string if there isn't any it means the player has won*/
void nRemover(char string[]);/*removes the \n */
void score(char name[] , int score);/*organizes the scores and writes them into the score file*/
void showScores();/*shows the scores at the end of the game*/

main(void)
{
    /*---------------------------------DECLARATION OF VARIABLES---------------------------------*/
    FILE* wordDic = NULL;
    char entered_char;
    char starWord[MAX] = "";
    char winPath[20] = ".\\wordDIC";
    char mysteryWord[MAX] = "";
    char ans = 0;
    char name[MAX] = "";
    int i = 0;
    int ran = 0; /*ran holds a random number that represents the number of lines*/
    int lifes = 10;/*initial number of lifes ... the numbers of shots to go*/
    int check = 0; /*checks if there is any more stars*/
    int Max = 0; /*max lines*/
    long scor = 0; 
    const int Min = 0;
    time_t  secA = 0 , secB = 0;
    /*------------------------------------------------------------------------------------------*/
    do
    {
#ifdef LINUX /*linux version*/
        wordDic = fopen("wordDIC" , "r");
        if(wordDic == NULL)
        {
            perror("wordDIC");
            return -1;
        }
#endif
#ifdef WINDOWS /*windows version*/
        wordDic = fopen(winPath , "r");
        if(wordDic == NULL)
                {
                        perror("wordDIC");
                        return -1;
                }

#endif
        /*----------INITIALIZING----------------------------------*/
        scor = 0;
        lifes = 10;
        Max = wordCalculator();
        srand(time(NULL));
        ran = (rand() % (Max - Min + 1)) + Min;
        /*-------------------------------------------------------*/
        for(i = 0 ; i < ran ; i++)
        {
            fgets(mysteryWord , sizeof(mysteryWord) , wordDic);
            nRemover(mysteryWord);
        }
/*now mystery word has the random word*/
        starMaker(mysteryWord , starWord);
        printf("Enter your name : ");/*entering the pseodo*/
        fgets(name , sizeof(name) , stdin);
        nRemover(name);
        time(&secA);/*start counting seconds*/
        while(lifes > 0 && (check = starFinder(starWord)) == FOUND)
        {
            printf("Word : %s\n" , starWord);
            printf("Enter a character (%d shots left) : \n" , lifes);
            entered_char = getchar();
            getchar();
            entered_char = toupper(entered_char);
            if(!queryFinder(entered_char , mysteryWord , starWord))
                lifes--;
            system("clear");
        }
        time(&secB);/*stop counting*/
        scor = secB - secA; /*calculating the score*/
        if(lifes == DEAD)
            printf("YOU HAVE LOST \n");
        else
        {
            printf("CONGRATS YOU HAVE WON , THE WORD IS INDEED %s , YOU FOUND THE WORD in %ld SECONDS\n" , mysteryWord , scor);
            score(name , scor);
        }
        printf("Would you like to play another game ? y/n : ");
        ans = getchar();
        getchar();
        fclose(wordDic);
    }
    while(ans == 'y' || ans == 'Y');
    printf("would you like to see the scores ? y/n : ");
    ans = getchar();
    getchar();
    if(ans == 'y' || ans == 'Y')
        showScores();
}
void starMaker(char string[] , char starContainer[])
{
    int i;
    int lenght = strlen(string);
    for(i = 0 ; i < lenght ; i++)
        starContainer[i] = '*';

    starContainer[i] = '\0';
}
int wordCalculator()
{
    FILE* wordDic = NULL;
    char buff[1000];
    int i = 0;
    wordDic = fopen("wordDIC" , "r");
    if(wordDic == NULL)
                {
                        perror("wordDIC");
                        return -2;
                }
    while(fgets(buff , sizeof(buff) , wordDic) != NULL)
        i++;
    fclose(wordDic);
    return i;
}
int queryFinder(char letter , char string[] , char starWord[])
{
    int i;
    int lenght = strlen(string);
    if(!strchr(string , letter))
        return NOT_FOUND;
    else
        for(i = 0 ; i < lenght ; i++)
        {
            if(string[i] == letter)
                starWord[i] = toupper(letter);
        }
}
int starFinder(char starWord[])
{
    if(!strchr(starWord , '*'))
        return NOT_FOUND;
    else
        return FOUND;
}
void nRemover(char string[])
{
    int i = 0;
    int lenght = strlen(string);
    for(i = lenght ; i > 0 ; i--)
        if(string[i] == '\n')
            string[i] = '\0';
}
void score(char name [] , int score)
{
    FILE* fscore = NULL;
    fscore = fopen("score" , "a+");
    if(fscore == NULL)
    {
        perror("score");
        exit(-10);
    }
    fprintf(fscore , "%25s **** %3d seconds\n" , name , score);
    fclose(fscore);
}
void showScores()
{
    char scoreLine[MAX] = "";
    FILE* scoreFile = NULL;
    scoreFile = fopen("score" , "r");
    if(scoreFile == NULL)
    {
        perror("score");
        exit(-4);
    }
    while(fgets(scoreLine , sizeof(scoreLine) , scoreFile) != NULL)
    {
        printf("%s" , scoreLine);
    }
    fclose(scoreFile);
}
\$\endgroup\$
5
\$\begingroup\$

Don't use macros to conditionally change code.
Use the macros to define appropriate macros that can be used in code:

#ifdef LINUX /*linux version*/
        wordDic = fopen("wordDIC" , "r");
        if(wordDic == NULL)
        {
            perror("wordDIC");
            return -1;
        }
#endif
#ifdef WINDOWS /*windows version*/
        wordDic = fopen(winPath , "r");
        if(wordDic == NULL)
                {
                        perror("wordDIC");
                        return -1;
                }

#endif

This should be re-written as:

        wordDic = fopen(WORD_DICTIONARY , "r");
        if(wordDic == NULL)
        {
            perror("wordDIC");
            return -1;
        }

Then in the header you can define:

#if defined(LINUX)
#define WORD_DICTIONARY      "wordDIC"
#elif definded(WINDOWS)
#define WORD_DICTIONARY      ".\\wordDIC"
#else
#error "Unsupported platform"
#endif
\$\endgroup\$
  • \$\begingroup\$ thank you for your quick and very helpful answer I'll make sure to apply that in my future projects \$\endgroup\$ – apprentice Jul 19 '13 at 3:55
5
\$\begingroup\$
  • You could eliminate the function prototypes by defining main() lastly, cutting down on some code. In this way, main() will already be aware of the other functions in the same way with prototypes.

  • Keeping a "list of variables" could make maintenance more difficult as it won't be easy to, for instance, tell if a variable is still in use. In order to avoid this, declare or initialize variables as close to their use as possible. This is especially problematic in main().

  • This function:

    int starFinder(char starWord[])
    {
        if(!strchr(starWord , '*'))
            return NOT_FOUND;
        else
            return FOUND;
    }
    

    can just have a one-line ternary statement:

    int starFinder(char starWord[])
    {
        return (!strchr(starWord , '*')) ? NOT_FOUND : FOUND;
    }
    

    In addition, consider swapping the conditionals, which may sound more logical as the function is first trying to find something. If you make this change, you may also have to adjust the calling code.

    int starFinder(char starWord[])
    {
        return (strchr(starWord , '*')) ? FOUND : NOT_FOUND;
    }
    

Overall, this code isn't quite easy to follow. Some of the function names aren't too clear and it looks like more code can be still shortened or removed. Apart from the aforementioned advice, it could help to remove some unneeded comments, such as the "initializing block" in main().

Regarding the "initialization block" thing, there may be a code organization problem if you have to isolate a block of code with comments like that. If you're writing too much code in one function, then it may mean that it's doing too much or it's doing something that can be done in another function. In the case of main(), it should (ideally) do as little as possible, primarily calling other functions to do the real work.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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