3

I have been researching like mad while I learning C. I have been debugging a C program and I thought I had some major issues here. Now I have critical issues. I made a dummy program to print two strings in one statement as follows:

   #include<stdio.h>

int main(int argc, char* argv[])
{
    char *herp = "Derp";
    char *derp = "Herp";

    printf("Herp %s Derp %s\n", herp, derp);

    return 0;
}

This prints out as expected. I get

Herp Derp Derp Herp

So, I thought, let me debug my own program by doing something similar. The following line in my program

printf("word is: %s and jumbled word is: %s\n", word, jumbleWord);

Should print out something like

Word is: word and jumbled word is: dowr

But it prints out something like

and jumbled word is: dowr

Where did the first part of the output go? I need to be able to print these both on the same line to debug. Also, the fact that a statement like this is not working tells me that really weird things are going on and I am bald from tearing my hair out. As my linked posts indicates, I would eventually like to compare these string values, but how can I even do that if printf() is not working right?

I am posting the entire program below so you can see where everything is happening. I am just learning how to use pointers. I originally had two pointers point at the same memory when I wanted to jumble a word and that didn't work very well! So I fixed that and got two separate memory spaces with the words I need. Now, I just can't print them. This all makes sense given the code below:

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

#define MAX_WORD_LENGTH 25

//Define global variables 
int numWords; 

//Preprocessed Functions 
void jumblegame();
void readFile(char *[]);
void jumbleWord(char *);
void guess(char *,char *); 

int main(int argc, char* argv[])
{
    jumblegame();
    return 0;
}

void jumblegame()
{
    //Load File 
        int x = 5050; //Rows
        char *words[x];
        readFile(words);

    //Define score variables 
        int totalScore = 0;
        int currentScore = 0; 

   //Repeatedly pick a random work, randomly jumble it, and let the user guess what it is
         srand((unsigned int)time(NULL));
         int randomNum = rand() % numWords + 1;

         char source[MAX_WORD_LENGTH + 1];
         char jumble[MAX_WORD_LENGTH + 1];

         strncpy(source, words[randomNum], MAX_WORD_LENGTH + 1);
         strncpy(jumble, words[randomNum],MAX_WORD_LENGTH + 1);

         jumbleWord(jumble);

         guess(source, jumble);
         //printf("Random word is: %s\n ", words[randomNum]);
         //randomly jumble it           
}

void readFile(char *array[5049]) 
{
    char line[256]; //This is to to grab each string in the file and put it in a line. 
    int z = 0; //Indice for the array

    FILE *file;
    file = fopen("words.txt","r");

    //Check to make sure file can open 
    if(file == NULL)
    {
        printf("Error: File does not open.");
        exit(1);
    }
    //Otherwise, read file into array  
    else
    {
        while(!feof(file))//The file will loop until end of file
        {
           if((fgets(line,256,file))!= NULL)//If the line isn't empty
           {
             int len = strlen(line); 
             if (len > 0 && line[len - 1] == '\n') line[len - 1] = '\0';
             array[z] = malloc(strlen(line) + 1);
             strcpy(array[z],line);
             z++;
           }    
        }
    }
    fclose(file);
    numWords = z; 
}

void jumbleWord(char *word)
{
    int wordSize = strlen(word) - 1; 
    //durstenfeld Implementation of Fischer-Yates Shuffle
        int i; 
        int j; 
        char temp;
        for(i = wordSize - 1; i > 0; i--)
        {
            j =  rand() % (i + 1);
            temp = word[j];
            word[j] = word[i];
            word[i] = temp;
        }
}

void guess(char *word, char *jumbleWord)
{
     printf("original word is: %s\n", word);
     printf("jumbled word is: %s\n", jumbleWord);
     printf("source is: %s and jumbled word is: %s\n", word, jumbleWord);
}

I think most people at this point would burn C and slap themselves for sucking so bad at it. However, I am going to keep trucking along. So let me apologize for any derp, but please know that I have spent many hours probably being really stupid and staring at this. I would love to say, "Hey, C is so stupid because it won't do what I am telling it to". Unfortunately, I can't believe this. I think it is doing exactly what I am telling it to do, but I am too close to this problem to see what I am doing wrong.

As always, thank you for your help. My deepest respect, GeekyOmega

Community
  • 1
  • 1
GeekyOmega
  • 1,235
  • 6
  • 16
  • 34
  • 1
    You shouldn't call `srand` before each `rand`, but rather once at the start of the program. – chris Aug 01 '12 at 19:55
  • 1
    Side note: `char *jumbledWord= jumbleWord(jumble);`, you `jumbleWord` changes the string given as its argument itself. So `jumbleWord` and `jumble` will be the exact same thing after this call. – Shahbaz Aug 01 '12 at 19:56
  • Thanks Chris. I fixed that. Shahbaz, will fix that. I thought of a way where this might be redundant. As I said, new to pointers. :-) Still though, getting that bizarre problem. :-( – GeekyOmega Aug 01 '12 at 20:00
  • 1
    Arrays are 0 based in C so the line `int randomNum = rand() % 5049 + 1;` is producing an off-by-one error when using `randomNum` as an index into `words[]`. – Dave Rager Aug 01 '12 at 20:00
  • Thank you all so much for answering my questions AND THEN going beyond the call of duty to really help point out bad coding practice. You are the best group of people around. I was really tearing my hair out on this stuff and learned so much from you all. + rep for you all and if I knew you in person, would buy you a round of beer if you wanted. – GeekyOmega Aug 01 '12 at 21:41

2 Answers2

14

You have a carriage return '\r' at the end of the word(s).

Carriage returns move the write cursor to the left of the screen, so it is writing it, but it's overwriting what was already there.

Wug
  • 12,956
  • 4
  • 34
  • 54
Doug Currie
  • 40,708
  • 1
  • 95
  • 119
  • 1
    Apparantly he created the file on a Windows machine. – moooeeeep Aug 01 '12 at 20:01
  • 2
    I added an explanation of why that's significant. Hope you don't mind. – Wug Aug 01 '12 at 20:03
  • should I if statements then to get rid of that? Like the following code: int len = strlen(line); if(len > 0 && line[len - 1] == '\r') line[len - 1] = ''; ? Would that fix this problem? – GeekyOmega Aug 01 '12 at 20:48
  • 1
    `if (len > 0 && (line[len-1] == '\n' || line[len-1] == '\r')) { line[len-1] = '\0'; if (len > 1 && line[len-2] == '\r') line[len-2] = '\0';` } – Doug Currie Aug 01 '12 at 21:19
  • 2
    You can also use `strchr` to find the first `'\r'` and if one is found, replace with `'\0'`; this is a bit easier with gnu libc's `strchrnul` – Doug Currie Aug 01 '12 at 21:26
  • That got it sir! Thank you very much! The second line was clever in case \r was hiding a line before \n. Also, subtle but equally nice was your conditional len > 1. I created a function called "clean" and put this logic in it. In the future, with strings, I probably will copy this function into those programs to clean my strings of junk I don't want. – GeekyOmega Aug 01 '12 at 21:27
2

Does your words file actually have 5049 entries? Even if it doesn't, you shouldn't assume that it does. Your readFile function should determine how many words are actually present in your words array after reading the file, otherwise, taking a random index into the words array will result in accessing uninitialized strings, which I suspect is leading to your memory corruption.

So, if readFile only sees 10 words, you should only be choosing a random word between indices 0..9. You're already keeping the number of words in variable z in the readFile method, share that with the rest of your code.

When you choose a random index into an array, note that since the arrays are 0-based, you should get the random and just mod by the number of valid array elements. So use int randomNum = rand() % 5049; without the +1.

Furthermore, all this strlen/+1/-1 stuff is unnecessary and confusing, prefer strncpy over memcpy (and strcpy) for strings and you don't need to deal with that. Note that the result ofstrlen doesn't include the null terminator, so you won't need to account for that with -1. I think that your jumbleWord function should always be ignoring the last char in the jumble.

Use this strategy for your string allocations: you have MAX_WORD_LENGTH, declare strings in your game as either char word[MAX_WORD_LENGTH + 1] or char *word = malloc(MAX_WORD_LENGTH + 1). Now to copy strings, use strncpy(src, dest, MAX_WORD_LENGTH).

pb2q
  • 58,613
  • 19
  • 146
  • 147
  • I know that my readfile has only 5049 entries and wont' change. So I confess, yes, I hacked it. I could do while(!feof(file)){} and do a fgets to read it. – GeekyOmega Aug 01 '12 at 20:04
  • 1
    in that case, see the note about being off-by-one with rand. but you should still persist the actual number of words read, it isn't hard, and prevents potential errors with short files – pb2q Aug 01 '12 at 20:05
  • 1
    when I run your code with only **2** changes: keeping a global `numWords == z` set in `readFile`, and picking a rand with `int randomNum = rand() % numWords;`, the code runs fine, except that the last `char` is never jumbled, see my answer about that. There are probably other problems here though, hard to tell. But that should get you going. – pb2q Aug 01 '12 at 20:13
  • 1
    Note that `strncpy()` can be dangerous too. `strncpy(dst, src, strlen(src))` will _never_ null terminate `dst`. To be certain you should do `strncpy(dst, src, strlen(src) + 1)` or specifically null terminate `dst` in another step. – Dave Rager Aug 01 '12 at 20:18
  • 1
    @DaveRager natch, but I think that's outside the scope of the question: he'll get to that eventually. I have an answer in the system re: this issue: http://stackoverflow.com/questions/10943033/why-are-strings-in-c-usually-terminated-with-0/10943040#10943040 – pb2q Aug 01 '12 at 20:21
  • 1
    @pb2q right, I just thought it might be important to point out. In your answer you talk about `strlen +1/-1` being unnecessary and then point to `strncpy` so he doesn't need to deal with it. IMO `strncpy` is one place where `strlen + 1` is definitely necessary. – Dave Rager Aug 01 '12 at 20:26
  • 1
    @DaveRager definitely, thanks, and you're right it is relevant to my answer. I just thought it could lead to confusion to address it, but you've brought me around, I'll add a note. lemme know if it is less than useful, or go ahead and edit the answer. I prefer using e.g. `malloc(MAX_STR_LEN + 1)` everywhere. – pb2q Aug 01 '12 at 20:30
  • Wow. This stuff is, I think for a lack of better word, pro sexy code skills. The beauty is in the basics. I think my brain is wrapped around the idea of anything string related for strings. If I don't know how big the file is ahead of time and I need to put my file into the array? How can I define a array like array[numWords] before hand, or will I have to loop twice in readFile()? This is a style question. – GeekyOmega Aug 01 '12 at 20:43
  • 1
    @pb2q I think that approach is fine though if `strlen(dest) == MAX_WORD_LENGTH` that use of `strncpy` won't null terminate. It should be `MAX_WORD_LENGTH + 1` so `strncpy` reads the terminating null from `dest` if it happens to be `MAX_WORD_LENGTH` long. – Dave Rager Aug 01 '12 at 20:44
  • 1
    if you don't want to assume an upper-bound on the number of words, you'll need to use `malloc` for the words array, then `realloc` to grow your words array if necessary as you're reading the file. But don't worry about that at this point, just assume that you'll only read up to the first `MAX_WORDS`, and add a test to just drop out of your `readFile` loop if there are more. – pb2q Aug 01 '12 at 20:46
  • That got the print part. Doug Currie provided some extra logic to what I had and I created a function to "clean" my strings. Feeling pretty cool about this program now. – GeekyOmega Aug 01 '12 at 21:28
  • Unix/Linux uses a newline `\n` character to specify the end of a line. When printing to a terminal or printer this will cause an implicit carriage return `\r` followed by a new line. Windows inherited from DOS (my history could be somewhat lacking here, correct me where I'm wrong...) that lines are terminated by two characters, the explicit `\r` followed immediately by `\n`. In Windows when opening a file that was created on Unix, using `fopen()` without specifying binary mode, it will automatically convert the single `\n` to the `\r\n` pair. – Dave Rager Aug 01 '12 at 21:42