Thread: I don't know why my program is crashing

Hi all, I'm having a problem with the following code for a class assignment. The assignment is to create a 2 player word guessing game using strings.

This code is mainly C but uses a few C++ functions which we are allowed. The code compiles, however, it crashes after asking for the secret word. Any ideas of what I can do to fix that?

Code:

#include <stdio.h>
#include <string.h>
#include <ctype> 


/* Prints welcome message */
void printwelc()
{
    printf("Welcome to Word Guess!\n");
    
    return;
}


/* Gets a secret word from the second player */
void getword(char secret[], int size)
{
    int i;
    
    do
    {
        printf("Player 1, Please enter a secret word no larger than 16 characters\n");
        fgets(secret, size, stdin);
        secret[strlen(secret) - 1] = '\0';
        
        for (int i=0; secret[i]; i++) 
        {
            secret[i] = tolower(secret[i]);
        }
        
        if(strlen(secret) < 2)
        {
            printf("The word must be two or more letters\n");
        }
        else
            if(secret[i] < 'a' || secret[i] > 'z')
            {
                printf("The word must only contain letters\n");
            }
    }while((strlen(secret) < 2) || (secret[i] < 'a' || secret[i] > 'z'));
    
    return;
}


/* Copies the secret word into another string to be hidden*/
void copyword(char secret[], char hide[], int size)
{
    if(size >= (strlen(secret) + 1))
        {
            strcpy(hide, secret);
        }
    else
        {
            printf("Not enough room to copy!\n");
        }
    
    return;
}


/* Turns copied string into '_' for every letter */
void hideword(char hide[])
{
    int j;
    int length = strlen(hide);


    for (j=0; j < length; j++) 
    {
        hide[j] = '_';
    }
    
    return;
}


/*Gets a letter from the Player if he chooses to guess a letter*/
void guessletter(char hide[], char letguess[], int size, int size2, int num_lives)
{
    int k;
    
    do
    {
        printf("Number of lives: %d\n", num_lives);
        printf("Guess a letter in the word ");
        printf("%s\n", hide);
        
        fgets(letguess, size2, stdin);
        letguess[strlen(letguess) - 1] = '\0';
        
        for (int k=0; letguess[k]; k++) 
            {
                letguess[k] = tolower(letguess[k]);
            }
            
            if(strlen(letguess) > 1)
            {
                printf("You can only guess one letter at a time!\n");
            }
            else
                if(letguess[k] < 'a' || letguess[k] > 'z')
                {
                    printf("The guess must be a letter\n");
                }
    }while((strlen(letguess) > 1) || (letguess[k] < 'a' || letguess[k] > 'z'));
    
    return;
}




/*Check whether word contains letguess*/
/*If this doesn't work try changing it to a char and putting in an assumption*/
void rightorwrong(char secret[], char hide[], char letguess[], int size, int size2, int num_lives, int count, int length) 
{    
    int l;


    for (l=0; l < length; l++)
    if ((letguess[1] == secret[l]) && (hide[l] == '_')) 
    {
        hide[l] = letguess[1];
        count++;
    }
    else 
    {
        num_lives--;
    }
    
    return;
}


/* Tells Player if they have won or lost*/
void winorlose(int num_lives, char secret[])
{    
    if (num_lives == 0)
        printf("Sorry, you're hanged! The word is \"%s\"\n", secret);
    else 
        printf("Congratulations! The word is \"%s\"\n", secret);
    
    return;
}


int main()
{
    const int SIZE = 16;
    const int SIZE2 = 2;
    char secret[SIZE] = {'\0'};
    char hide[SIZE] = {'\0'};
    char letguess[SIZE2] = {'\0'};
    int num_lives = 10;
    int count = 0;
    int length = strlen(secret);
    
    printwelc();
    getword(secret, SIZE);
    copyword(secret, hide, SIZE);
    hideword(hide);
    do 
    {
        guessletter(hide, letguess, SIZE, SIZE2, num_lives);
        rightorwrong(secret, hide, letguess, SIZE, SIZE2, num_lives, count, length);
    } while ((num_lives != 0) && (count != length));
    winorlose(num_lives, secret);
    
    return 0;
}

This code is mainly C but uses a few C++ functions which we are allowed.

So ... exactly which one are you technically supposed to be learning? C and C++ are different languages. Sure, C++ is a superset of C, but despite sharing a similiar syntax, they each have their own way of doing things, and their own rules (libraries, programming paradigms, etc).

The code compiles...

Not for me. It complained about this line:

Code:

#include <ctype>

Depending on which language you're learning, this could be a few different things. I'm assuming the one you wanted was <ctype.h>.

It had a much easier time trying to compile as C++, so I'll just assume this is C++ written as C (which, for reasons touched on above, is generally not good practice).

...however, it crashes after asking for the secret word. Any ideas of what I can do to fix that?

First, check the warnings (and errors) you get from the compiler. This is from after changing the header to <ctype.h>:

Code:

/*
main.cpp||In function 'void getword(char*, int)':|
main.cpp|36|warning: name lookup of 'i' changed|
main.cpp|18|warning:   matches this 'i' under ISO standard rules|
main.cpp|26|warning:   matches this 'i' under old rules|
main.cpp||In function 'void copyword(char*, char*, int)':|
main.cpp|49|warning: comparison between signed and unsigned integer expressions|
main.cpp||In function 'void guessletter(char*, char*, int, int, int)':|
main.cpp|102|warning: name lookup of 'k' changed|
main.cpp|81|warning:   matches this 'k' under ISO standard rules|
main.cpp|92|warning:   matches this 'k' under old rules|
main.cpp|102|warning: 'k' may be used uninitialized in this function|
main.cpp||In function 'void getword(char*, int)':|
main.cpp|36|warning: 'i' may be used uninitialized in this function|
||=== Build finished: 0 errors, 9 warnings ===|
*/

One problem is the multiple declarations of variables, which is complicated by scope (see here for an illustration). For instance, the variable 'i' on line 18 is different from the variable 'i' in the "for()" loop on line 26.

Overall, after just studying the "getword()" function, I see logic errors that make me need to read no further. Let's analyze the code in that function.

Line 16: Enter the function
Line 18: Declare (but not initialize) a variable 'i'
Line 20: Start a do-while loop
Line 26: Run an inner loop that converts each character to its lower-case ... note the local variable 'i' declared within this loop (not the same 'i' as from Line 18)
Line 31: Check the length of the string for appropriate number of letters
Line 35: Otherwise, check that a character is <'a' or >'z' - but which variable 'i' is being used here? And what value does it have? And why is that code not within a loop rolling 'i'?
Line 40: While the length of the word is too short ... or a character is <'a' or >'z' - but again, which variable 'i' is being used here? And again, what value does it have here? Once again, that variable 'i' is not being looped as expected.

You have logic here, but it's not quite right. Maybe working it out on paper, perhaps with a simple flow chart, will help you develop a more concise logic. Perhaps something along the lines of:

Code:

/*
Do
  Get string
  If length too short,
    Set error flag
  Else
    Run loop:
      Convert to lower-case
      If not a lower-case letter
        Set error flag
While error flag is set
*/

There's more work to be done, but that should be enough to get you started.

I'm pretty sure this line is actually cutting your array short by one. So if you are trying to write to or copy the full length it could crash.

Let's look at the line previous to the one you posted.

Code:

        fgets(secret, size, stdin);
        secret[strlen(secret) - 1] = '\0';

The fgets() function retrieves the end of line character and places it in the string, if there is room. That second line removes the last character from the string, hopefully the end of string character (it'll remove the end of string character if it exists, otherwise it'll remove the last character).

Now the question I have for you is why do you think the program could crash because of that second line? You have reduced the length of the string by one, but the size of the array remains the same.

Jim

For reference, a link to a good discussion on the topic of removing the newline after "fgets()" -> Scanning new line (start at post #9)

That said, it is indeed a mistake because fgets could store an empty string, upon which we get:

Right, I forgot about the possiblity of EOF being triggered before any characters were retrieved.

Furthermore, in the event that the newline was not stored, then replacing the last character of the string with a null character is almost certainly an unintended effect.

Also true as I also pointed out as well, altough I didn't mention it would probably not produce the desired outcome, you were much clearer on this point as well.

But reading Alpo's post it appears to me that he thinks the array size is decreasing.

I'm pretty sure this line is actually cutting your array short by one. So if you are trying to write to or copy the full length it could crash.

The size of the array is not altered, only the length of the string changes.

Jim

Thank you for the advice, I've been working on fixing my code but have run into another issue with my Do-While Loop. The errors work fine, however, the loop continues even when there are no error flags.

Code:

/* Gets a secret word from the second player */
void getword(char secret[], int size)
{
	int i;
	
	do
	{
		printf("Player 1, Please enter a secret word no larger than 16 characters\n");
		fgets(secret, size, stdin);
		secret[strlen(secret) - 1] = '\0';
		
		if(strlen(secret) < 2)
		{
			printf("The word must be two or more letters\n");
		}
		else
		{	
			for (i=0; i < strlen(secret); i++) 
			{
				secret[i] = tolower(secret[i]);
		
				if(secret[i] < 'a' || secret[i] > 'z')
				{
					printf("The word must only contain letters\n");
				}	
			}
		}
	}while((strlen(secret) < 2) || (secret[i] < 'a' || secret[i] > 'z'));
	
	return;
}

You're trying to use "secret[i]" as the argument to your "while()", but 'i' does not have a meaningful value there.

As hinted at in post #2, I would recommend creating an additional variable to act as an error flag. Set this flag during any failed condition checks within the loop, and simply use this flag to control the loop.

Apart from that, it looks like you've implement all the other suggestions quite well. And the code is neat and well formatted. Good work!

Thanks for the help, I've fixed up that while loop, however, I have run into another issue. The following function keeps on looping when there aren't any errors, also the more letters I put in for the guess the more prompts are printed out:

Code:

/*Gets a letter from the Player if he chooses to guess a letter*/
void guessletter(char hide[], char letguess[], int size, int size2, int num_lives, int error)
{
    do
    {
        error = 0;
        
        printf("Number of lives: %d\n", num_lives);
        printf("Guess a letter in the word ");
        printf("%s\n", hide);
        
        fgets(letguess, size2, stdin);
        letguess[strlen(letguess) - 1] = '\0';
        
        if(strlen(letguess) > 1)
        {
            printf("You can only guess one letter at a time!\n");
            error++;
        }
        else
        {
            letguess[0] = tolower(letguess[0]);
            
            if(letguess[0] < 'a' || letguess[0] > 'z')
            {
                printf("The guess must be a letter\n");
                error++;
            }
        }
    }while(error != 0);
    
    return;
}

Sorry I'm asking for help again, but I've tried everything I can think of.

This is the current code:

Code:

/* currently goes up to get a letter then quits*/
#include <stdio.h>
#include <string.h>
#include <ctype.h> 


/* Prints welcome message */
void printwelc()
{
	printf("Welcome to Word Guess!\n");
	
	return;
}


/* Gets a secret word from the second player */
/*works*/
void getword(char secret[], int size, int error)
{
	int i;
	
	do
	{
		error = 0;
		
		printf("Player 1, Please enter a secret word no larger than 16 characters\n");
		fgets(secret, size, stdin);
		secret[strlen(secret) - 1] = '\0';
		
		if(strlen(secret) < 2)
		{
			printf("The word must be two or more letters\n");
			error++;
		}
		else
		{	
			for (i=0; i < strlen(secret); i++) 
			{
				secret[i] = tolower(secret[i]);
		
				if(secret[i] < 'a' || secret[i] > 'z')
				{
					printf("The word must only contain letters\n");
					error++;
				}	
			}
		}
	}while(error != 0);
	
	return;
}


/* Copies the secret word into another string to be hidden*/
/*works*/
void copyword(char secret[], char hide[], int size)
{
	if(size >= (strlen(secret) + 1))
		{
			strcpy(hide, secret);
		}
	else
		{
			printf("Not enough room to copy!\n");
		}
	
	return;
}


/* Turns copied string into '_' for every letter */
/*works*/
void hideword(char hide[])
{
	int j;
	int lengt = strlen(hide);


	for (j=0; j < lengt; j++) 
	{
		hide[j] = '_';
	}
	
	return;
}


/*Gets a letter from the Player if he chooses to guess a letter*/
/* re-prints prompt*/
void guessletter(char hide[], char letguess[], int size, int size2, int num_lives, int error)
{
	do
	{
		error = 0;
		
		printf("Number of lives: %d\n", num_lives);
		printf("Guess a letter in the word ");
		printf("%s\n", hide);
		
		fgets(letguess, size2, stdin);
		letguess[strlen(letguess) - 1] = '\0';
		
		if(strlen(letguess) > 1)
		{
			printf("You can only guess one letter at a time!\n");
			error++;
		}
		else
		{
			letguess[0] = tolower(letguess[0]);
			
			if(letguess[0] < 'a' || letguess[0] > 'z')
			{
				printf("The guess must be a letter\n");
				error++;
			}
		}
	}while(error != 0);
	
	return;
}




/*Check whether word contains letguess*/
/*If this doesn't work try changing it to a char and putting in an assumption*/
void rightorwrong(char secret[], char hide[], char letguess[], int size, int size2, int num_lives, int count, int length) 
{	
	int l;
	int m;


	for (l=0; l<length; l++) 
	{
		if (letguess[0] == secret[l])
		{
			for (m=0; m<length; m++)
			{
				if ((letguess[0] == secret[m]) && (hide[m] == '_')) 
				{
					hide[m] = letguess[0];
					count++;
				}
			}
		}
		else 
		{
			num_lives--;
		}
	}
	
	return;
}


/* Tells Player if they have won or lost*/
void winorlose(int num_lives, char secret[])
{	
	if (num_lives == 0)
	{
		printf("Sorry, you're hanged! The word is \"%s\"\n", secret);
	}
	else
	{
		printf("Congratulations! The word is \"%s\"\n", secret);
	}
	
	return;
}


int main()
{
	const int SIZE = 16;
	const int SIZE2 = 2;
	char secret[SIZE] = {'\0'};
	char hide[SIZE] = {'\0'};
	char letguess[SIZE2] = {'\0'};
	int num_lives = 10;
	int count = 0;
	int length = strlen(secret);
	int error;
	
	printwelc();
	getword(secret, SIZE, error);
	copyword(secret, hide, SIZE);
	hideword(hide);
	do 
	{
		guessletter(hide, letguess, SIZE, SIZE2, num_lives, error);
		rightorwrong(secret, hide, letguess, SIZE, SIZE2, num_lives, count, length);
	}while((num_lives != 0) && (count != length));
	winorlose(num_lives, secret);
	
	return 0;
}

For the secret word I input 'apple', when prompted for a letter I input 'a' I then get the message 'The guess must be a letter'' the prompt then prints twice in a row. Even weirder, the more letters I guess for example 'aa' the more times the prompt prints E.g 'a' prompt prints twice, 'aa' the prompt prints three times.

Thanks again for all your help