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

  1. #16
    Registered User
    Join Date
    Jun 2011
    Posts
    4,513
    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''...
    Code:
    // size2 == 2
    
    fgets(letguess, size2, stdin);
    Quote Originally Posted by fgets()
    Reads characters from stream and stores them as a C string into str until (num-1) characters have been read or either a newline or the end-of-file is reached, whichever happens first.
    So only a single character (size2 - 1 = 1) is read. When you enter 'a' followed by a newline, 'a' is stored in the first element. "fgets()" puts a '\0' in the second.

    Code:
    letguess[strlen(letguess) - 1] = '\0';
    Then you blindly replace a character with '\0', without checking that you are overwriting a newline (which you're not, since there was no room to store it). The string length of "a" is one, so you're overwriting the first element with '\0', creating an empty string.

    '\0' fails the "check letter" test, hence the message you're seeing.

    ...then prints twice in a row.
    Anything not read by "fgets()" remains on the input buffer. So since only 'a' was read the first time around, '\n' was read the second time around (from the same initial input you gave). The "check letter" test fails again, and the message is printed out a second time.

    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.
    Once again, anything not read by "fgets()" remains on the input buffer. So when you enter a bunch of letters, one is read at a time, incorrectly processed (in fact, replaced with '\0'), the "check letter" test fails, and the message is printed out - for each character entered.

    Fortunately, the fix is pretty easy.

    1. Only set the last character to '\0' if the last character is a newline (see here for an example)
    2. If the last character is not a newline, run a loop to eat up all the left over characters on the input buffer (see here)

    [edit] It probably wouldn't hurt to make "SIZE2" a little bigger, so you're not limited to reading only a single character (this might make processing easier for you).
    Last edited by Matticus; 05-13-2014 at 09:40 AM.

  2. #17
    Registered User Alpo's Avatar
    Join Date
    Apr 2014
    Posts
    877
    I went through with a pencil and jotted down some things, not all of them are related to printing twice, some were just changes you can make to make things easier on yourself.

    line 41 && line 112)
    Code:
    if(secret[i] < 'a' || secret[i] > 'z')


    You can use the islower(array[I]) function from ctype.h to check for this condition.

    line 58)
    Code:
        if(size >= (strlen(secret) + 1))
    This is a redundant check, as size is 16, and the nature of strlen means that strlen + 1 will always be the total array indexes.


    lines 79 - 82)
    Code:
         for (j=0; j < lengt; j++) 
        {
            hide[j] = '_';
        } 
    It would be simpler to just use memset(hide, '_', strlen(hide)) .


    line 101)
    Code:
    letguess[strlen(letguess) - 1] = '\0';
    The previous call to fgets (line 100), is unlikely to contain a newline, this means letguess will only contain 1 letter. So strlen -1 is basically letguess[0].

    line 103)
    Code:
    if(strlen(letguess) > 1)


    Do to previously mentioned, even if you use simply strlen(letguess), this condition will always fail.

    line 191 & guessletter definition)
    Code:
     guessletter(hide, letguess, SIZE, SIZE2, num_lives, error);
    You aren't using SIZE, SIZE2, or num_lives in this function, the extra variables just cloud up attempted diagnosis of problems. You might want to use them for something later, but I would wait until you need them there to pass them.


    line 192)
    Code:
     rightorwrong(secret, hide, letguess, SIZE, SIZE2, num_lives, count, length);
    Similar problems of unused variables.


    line 177, 182 && 134)

    177:
    Code:
     char secret[SIZE] = {'\0'};
    182:
    Code:
    int length = strlen(secret);
    134:
    Code:
    for (l=0; l<length; l++)
    I put these side by side so you could see the logical flow of what the program is actually doing. The entire loop from 134 downward will never execute, because the condition on the first for loop is already satisfied. Edit for clarity: Strlen(secret) has no length, because it's first element is a null byte.


    That's about it, I can't say if you fix these everything will run smoothly, but you should be closer. Good luck!



    Last edited by Alpo; 05-13-2014 at 12:17 PM.

  3. #18
    Registered User
    Join Date
    May 2014
    Posts
    5
    Hi guys, I'm back with another issue. I've fixed a lot of my code and it's getting a lot closer to what I want. I've changed letguess into a char for simplicity and fixed my parameters so that everything passes properly.

    I'm having a problem with my num_lives variable, regardless of whether you guess a letter correctly or not you lose a life each turn. E.g I enter 'ape' for the word and guess 'a' as a letter, hide is changed so it now show 'a__' however the amount of lives has also decreased by 1.

    Another issue I'm having is when I enter a word longer than 3 characters, you automatically win after guessing 3. E.g I enter 'apple' for the word and guess 'a', then 'l', then when I enter 'e' I automatically win the game even though it's still missing two letters 'a__le'.

    My updated code is below, unfortunately I wasn't able to implement all of the suggestions as we're only allowed to use what we've learnt in the course. Once again a big thank you to everyone for their time and patience, I very much appreciate it.

    Code:
    #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 i;
    	int error1 = 0;
    	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");
    			error1++;
    		}
    		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");
    					error1++;
    				}	
    			}
    		}
    	}while(error1 != 0);
    	printf("length is %d", strlen(secret));
    	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*/
    /* breaks after one invalid option*/
    char guessletter(char hide[], int size, int num_lives)
    {
    	int error2 = 0;
    	char letguess;
    	
    	do
    	{	
    		printf("Number of lives: %d\n", num_lives);
    		printf("Guess a letter in the word ");
    		printf("%s\n", hide);
    		scanf("%c%*c", &letguess);
    		
    		letguess = tolower(letguess);
    		
    		if(letguess < 'a' || letguess > 'z')
    		{
    			printf("The guess must be a letter\n");
    			error2++;
    		}
    		
    	}while(error2 != 0);
    	
    	return(letguess);
    }
    
    
    
    
    /*Check whether word contains letguess*/
    /*takes 1 life every turn regardless if you got it right or not*/
    /* it doesn't work for words over 3 letters*/
    void rightorwrong(char secret[], char hide[], char letguess, int size, int &num_lives, int &count, int length) 
    {	
    	int l;
    	int m;
    	int guess;
    	char let = letguess;
    	char livepass = 0;
    
    
    	for (l=0; l<=length; l++) 
    	{
    		if (let == secret[l])
    		{
    			for (m=0; m<=length; m++)
    			{
    				if ((let == secret[m]) && (hide[m] == '_')) 
    				{
    					hide[m] = let;
    					count++;
    					printf("l is %d, m is %d, letguess is %c, secret[m] is %c, hide[m] is %c\n ", l, m, letguess, secret[m], hide[m]);
    				
    				}
    			}
    		}
    		else 
    			if (let != secret[l])
    			{
    				livepass++;
    			}
    	}
    	
    	if(livepass > 0)
    	{
    		num_lives--;
    	}
    	
    	return;
    }
    
    
    
    
    /* Tells Player if they have won or lost*/
    /*Kind of works needs more testing after fixes*/
    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;
    	char secret[SIZE];
    	char hide[SIZE];
    	int num_lives = 10;
    	int count = 0;
    	int length = strlen(secret);
    	char let;
    	
    	printwelc();
    	getword(secret, SIZE);
    	copyword(secret, hide, SIZE);
    	hideword(hide);
    	do 
    	{
    		let = guessletter(hide, SIZE, num_lives);
    		rightorwrong(secret, hide, let, SIZE, num_lives, count, length);
    	}while((num_lives > 0) && (count != length));
    	winorlose(num_lives, secret);
    	
    	return 0;
    }

  4. #19
    Master Apprentice phantomotap's Avatar
    Join Date
    Jan 2008
    Posts
    5,108
    O_o

    If this is C++ as the last code implies, you should just do yourself a favor and use `std::string' and related paraphernalia.

    If this isn't C++, you need to remove code that is C++.

    Make your decision before we continue.

    Soma
    “Salem Was Wrong!” -- Pedant Necromancer
    “Four isn't random!” -- Gibbering Mouther

  5. #20
    Registered User Alpo's Avatar
    Join Date
    Apr 2014
    Posts
    877
    Just move the strlen until after getword(), you can't count on strlen for uninitialized array. Also, change the 2nd condition of your while loop to strcmp(hide, secret) != 0, it will be more reliable.

    Code:
    int main()
    {
        const int SIZE = 16;
        char secret[SIZE];
        char hide[SIZE];
        int num_lives = 10;
        int count = 0;
        int length;
        char let;
    
        printwelc();
        getword(secret, SIZE);
        length = strlen(secret);
        copyword(secret, hide, SIZE);
        hideword(hide);
    
        do {
            let = guessletter(hide, SIZE, num_lives);
            rightorwrong(secret, hide, let, SIZE, num_lives, count, length);
        }
        while((num_lives > 0) && strcmp(secret, hide) != 0);
        winorlose(num_lives, secret);
        return 0;
    }

    As for num_lives, my suggestion is to look at this line, really think about what is going on here:

    Code:
            else
                if (let != secret[l])
                {
                    livepass++;
                }
    


    Edit: Phantomotap - Are you talking about the const int SIZE, thing? That is still C isn't it?
    Last edited by Alpo; 05-15-2014 at 10:42 PM. Reason: Added spacing back in

  6. #21
    Master Apprentice phantomotap's Avatar
    Join Date
    Jan 2008
    Posts
    5,108
    O_o

    @Alpo: You should check the source with more care.

    To be fair, the C++ is nearly irrelevant which leads me to believe the poster is receiving help elsewhere from a C++ programmer.

    Soma
    “Salem Was Wrong!” -- Pedant Necromancer
    “Four isn't random!” -- Gibbering Mouther

  7. #22
    Registered User
    Join Date
    May 2009
    Posts
    4,183
    Quote Originally Posted by Alpo View Post
    Just move the strlen until after getword(), you can't count on strlen for uninitialized array. Also, change the 2nd condition of your while loop to
    Edit: Phantomotap - Are you talking about the const int SIZE, thing? That is still C isn't it? [/COLOR]
    IIRC, that line is allowed in C; but, the next line is NOT allowed in C.
    At least not the versions of C I use.
    Its the reason, I use MACROS instead of const too often in C++.

    Tim S.
    "...a computer is a stupid machine with the ability to do incredibly smart things, while computer programmers are smart people with the ability to do incredibly stupid things. They are,in short, a perfect match.." Bill Bryson

  8. #23
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    28,412
    Quote Originally Posted by stahta01
    IIRC, that line is allowed in C; but, the next line is NOT allowed in C.
    At least not the versions of C I use.
    Its the reason, I use MACROS instead of const too often in C++.
    Dunno, I think the more obvious C++ feature used would be reference parameters. After all, C99 or later has variable length arrays as standard.
    Quote Originally Posted by Bjarne Stroustrup (2000-10-14)
    I get maybe two dozen requests for help with some sort of programming or design problem every day. Most have more sense than to send me hundreds of lines of code. If they do, I ask them to find the smallest example that exhibits the problem and send me that. Mostly, they then find the error themselves. "Finding the smallest program that demonstrates the error" is a powerful debugging tool.
    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  9. #24
    Master Apprentice phantomotap's Avatar
    Join Date
    Jan 2008
    Posts
    5,108
    After all, C99 or later has variable length arrays as standard.
    O_o

    I'd agree with the "obvious", but I'd also say that is the problem.

    You can easily see subtle differences like that causing a newbie a lot of problems if a habit develops.

    *shrug*

    Even targeting C99, a lot of places forbid "VLA".

    Soma
    “Salem Was Wrong!” -- Pedant Necromancer
    “Four isn't random!” -- Gibbering Mouther

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Any help on why my program is crashing?
    By brookemay in forum C Programming
    Replies: 2
    Last Post: 11-19-2011, 12:54 PM
  2. Program keeps crashing
    By o.fithcheallaig in forum C Programming
    Replies: 25
    Last Post: 10-20-2011, 03:57 AM
  3. need help program crashing
    By tunerfreak in forum C++ Programming
    Replies: 14
    Last Post: 05-22-2006, 11:29 AM
  4. Program Crashing
    By Pressure in forum C Programming
    Replies: 3
    Last Post: 04-18-2005, 10:28 PM
  5. Program Crashing - please help!
    By Surfin_Bird in forum C Programming
    Replies: 6
    Last Post: 03-23-2005, 11:34 AM

Tags for this Thread