reading strings from .txt and entering in an array

I am trying to read a file of 500k+ words that will be entered into a binary search tree. First I am just trying to read these words into an array of strings but it does not seem to be working. Below is the code I wrote but after spending an hour figuring out why every entry is being set to the very last word in the .txt I am at a loss. Below the code is the output, I don't need the output but I was using it as an attempt to debug the code or to see what is going wrong... but it did not help.

Code:

---

int main() {
        FILE *file;
        int numWords=0, i, j;
        char fileName[30], wordString[30];
        char **wordArray;
        printf("Enter the name of the file:");
        scanf("%s", fileName);

        file = fopen(fileName, "r");
        if(!file) {
          printf("File does not exist.  Program terminated");
          return 1;
        }

        else {
          while(!feof(file)) {
            fgets(wordString, 30, file);
            numWords++;
          } rewind(file);
         
          wordArray = (char **)checked_malloc(numWords*sizeof(char*));
          for(i = 0; fgets(wordString, 30, file); ++i) {
            printf("%d %s", i, wordString);
            wordArray[i] = (char*)checked_malloc(strlen(wordString)*sizeof(char));
            wordArray[i] = wordString;
           
          } fclose(file);
        }

        printf("i=%d    words=%d  \n", i, numWords-1);
        for(j = 0; j < numWords-1; j++)
          printf("%d: %s ", j, wordArray[j]);

        free(wordArray);
        return 0;
}

---

Quote:

---

Enter the name of the file:small.txt
0 cat
1 mouse
2 bear
3 deer
4 haha
5 newbie
6 llama
7 dog
8 animal
9 rocket
10 ship
11 blanket
12 snake
13 ohnoes
14 wtfbbq
i=15 words=15
0: wtfbbq
1: wtfbbq
2: wtfbbq
3: wtfbbq
4: wtfbbq
5: wtfbbq
6: wtfbbq
7: wtfbbq
8: wtfbbq
9: wtfbbq
10: wtfbbq
11: wtfbbq
12: wtfbbq
13: wtfbbq
14: wtfbbq

---

As the output shows, every string in the array wordArray is set to the last word in the txt. If anyone could show me what part I am doing wrong it would be greatly appreciated.

Thanks.

What does this line do [and think about it in the sense of what the compiler makes of it, rather than what it looks like to you]:

Code:

---

wordArray[i] = wordString;

---

I guarantee, that this is exactly the only line you would need to change.

--
Mats

Not surprised it doesn't work...
OK, here goes...

Quote:

---

Originally Posted by trancekid

Code:

---

int main() {
        FILE *file;
        int numWords=0, i, j;
        char fileName[30], wordString[30];
        char **wordArray;
        printf("Enter the name of the file:");
/* Don't use scanf to read strings! It's unsafe! Recommend use fgets. */
        scanf("%s", fileName);

        file = fopen(fileName, "r");
        if(!file) {
          printf("File does not exist.  Program terminated");
          return 1;
        }

        else {
          while(!feof(file)) {
            fgets(wordString, 30, file);
            numWords++;
          } rewind(file); /* Come on, put that on a separate line, it's confusing */         

          wordArray = (char **)checked_malloc(numWords*sizeof(char*));
          for(i = 0; fgets(wordString, 30, file); ++i) {
/* Reading the contents of the file twice. Waste of time. Merge it into one loop and read them */
            printf("%d %s", i, wordString);
            wordArray[i] = (char*)checked_malloc(strlen(wordString)*sizeof(char));
            wordArray[i] = wordString; /* *cough* Read a tutorial on strings *cough */
           
          } fclose(file);
        }

        printf("i=%d    words=%d  \n", i, numWords-1);
        for(j = 0; j < numWords-1; j++)
          printf("%d: %s ", j, wordArray[j]);


/* Bad! Free the elements of the array separately, THEN free wordArray */
/* You allocated a char** array first, then EACH of that array allocated MORE memory, so you have a leak */
        free(wordArray);
        return 0;
}

---

---

Well, I kind of thought it was that line for a while but I guess my reasoning has failed me yet again. Anyways...

wordString is obviously the first, or next, word in the file. So I am allocating enough memory at wordArray[i] (since it is a double pointer, for an array of strings when a malloc string is just an array of characters) for the size of whatever this word may be. I assume that once wordArray[i] has been given the correct amount of memory, I can simply set it = to the string of characters that make up wordString... I guess this is not the case but I don't see where it is going wrong. Do I have to run another loop inside the fgets loop that enters each char of said string? Like...

for(j = 0; j < strlen(wordString); j++) {
wordArray[i][j] = wordString[j];

??

Yes, it's inefficient to read 500k lines of words. But it's also inefficient to realloc() many times over, if you don't get that part right.

And as usual, "It's easier to make a correct program run faster than to fix a fast program full of bugs" [paraphrasing here], so optimize later on if needs be.

--
Mats

Quote:

---

Originally Posted by trancekid

wordString is obviously the first, or next, word in the file. So I am allocating enough memory at wordArray[i] (since it is a double pointer, for an array of strings when a malloc string is just an array of characters) for the size of whatever this word may be. I assume that once wordArray[i] has been given the correct amount of memory, I can simply set it = to the string of characters that make up wordString... I guess this is not the case but I don't see where it is going wrong. Do I have to run another loop inside the fgets loop that enters each char of said string? Like...

---

http://www.cprogramming.com/tutorial/c/lesson9.html

Code:

---

int main() {
        FILE *file;
        int numWords=0, i, j;
        char fileName[30], wordString[30];
        char **wordArray;
        printf("Enter the name of the file:");
        scanf("%s", fileName);

        file = fopen(fileName, "r");
        if(!file) {
          printf("File does not exist.  Program terminated");
          return 1;
        }

        else {
          while(!feof(file)) {
            fgets(wordString, 30, file);
            numWords++;
          }
          rewind(file);  //just for you angry guy!!!
         
          wordArray = (char **)checked_malloc(numWords*sizeof(char*));
          for(i = 0; fgets(wordString, 30, file); ++i) {
            printf("%d %s", i, wordString);
            wordArray[i] = (char*)checked_malloc(strlen(wordString)*sizeof(char));
            for(j = 0; j < strlen(wordString); j++)
              wordArray[i][j] = wordString[j];
           
          } fclose(file);
        }

        printf("i=%d    words=%d  \n", i, numWords-1);
        for(j = 0; j < numWords-1; j++) {
          printf("%d: %s ", j, wordArray[j]);
          free(wordArray[j]);
        }

        free(wordArray);
        return 0;
}

---

Thank to both of you for the constructive criticism. It works like a charm now.

But the reason I am rewinding the file and reading everything a second time is because I need to know how many words are in the file to allocate the correct amount of space. Would you suggest I simply allocated a whole bunch of space then trim it down to the correct size by deleting all null pointers in the **?

Thanks again guys!

Quote:

---

Originally Posted by trancekid

Thank to both of you for the constructive criticism. It works like a charm now.

But the reason I am rewinding the file and reading everything a second time is because I need to know how many words are in the file to allocate the correct amount of space. Would you suggest I simply allocated a whole bunch of space then trim it down to the correct size by deleting all null pointers in the **?

Thanks again guys!

---

You could make a dynamic array. Guess how much space you need, allocate, read and if it turns out you don't have enough, use realloc to get more.
Read the string tutorial I gave you and also think about your allocation/free procedure, because you're leaking memory.

Oh, and you want to know what happens when you do:

Code:

---

char*p = "My string here";
char* p2 = p;

---

Hm? p's address is assigned to p2. The string isn't copied, so when p dies, p2 is invalid. Which turns out to be your case too (when you read a new line, the contents of your pointers change because it points to the address of your buffer).

And instead of making your own strcpy() implementation, why not use:

Code:

---

strcpy(wordArray[i], wordString);

---

Also, you don't free all the entries in wordArray, only the first allocation.

And there's another bug that both me and elysia missed: You are allocating strlen bytes, you need one more to store the zero at the end of the string.

--
Mats

Quote:

---

Originally Posted by matsp

And instead of making your own strcpy() implementation, why not use:

Code:

---

strcpy(wordArray[i], wordString);

---

Also, you don't free all the entries in wordArray, only the first allocation.

And there's another bug that both me and elysia missed: You are allocating strlen bytes, you need one more to store the zero at the end of the string.

--
Mats

---

When I do this...

Code:

---

        printf("i=%d    words=%d  \n", i, numWords-1);
        for(j = 0; j < numWords-1; j++) {
          printf("%d: %s ", j, wordArray[j]);
          for( i = 0; i < strlen(wordArray[j]); i++)
            free(wordArray[j][i]);
          free(wordArray[j]);
        }

---

I get this error...

Quote:

---

Dict.c:69: warning: passing argument 1 of ‘free’ makes pointer from integer without a cast

---

I also added one extra byte for the \0.

Code:

---

            wordArray[i] = (char*)checked_malloc(strlen(wordString)*sizeof(char)+1);

---

Quote:

---

Originally Posted by trancekid

When I do this...

Code:

---

        printf("i=%d    words=%d  \n", i, numWords-1);
        for(j = 0; j < numWords-1; j++) {
          printf("%d: %s ", j, wordArray[j]);
          for( i = 0; i < strlen(wordArray[j]); i++)
            free(wordArray[j][i]);
          free(wordArray[j]);
        }

---

I get this error...
I also added one extra byte for the \0.

---

Well, first it's a warning.
And duh... have a look at your allocation code:

Code:

---

          wordArray = (char **)checked_malloc(numWords*sizeof(char*));
            wordArray[i] = (char*)checked_malloc(strlen(wordString)*sizeof(char));

---

wordArray is allocated.
wordArray[i] is allocated.
But you're freeing:
wordArray[j][i]
wordArray[j]

Btw, wordChar[j][i] == char. It's a dereferenced pointer. You need to release the pointers, not the data.

well then duh it was right the second time I fixed it.

Code:

---

        for(j = 0; j < numWords-1; j++) {
          printf("%d: %s ", j, wordArray[j]);
          free(wordArray[j]);
        }

        free(wordArray);
        return 0;

---

The thing is, it's not.
Do the reverse of your allocation. Replace malloc with free.
Free every element in wordArray. From wordArray[0] to wordArray[numWords - 1].
THEN free wordArray itself.
You're just freeing the FIRST element of wordArray.

Quote:

---

Originally Posted by Elysia

The thing is, it's not.
Do the reverse of your allocation. Replace malloc with free.
Free every element in wordArray. From wordArray[0] to wordArray[numWords - 1].
THEN free wordArray itself.
You're just freeing the FIRST element of wordArray.

---

Code:

---

        for(j = 0; j < numWords-1; j++) {
          printf("&#37;d: %s ", j, wordArray[j]);
          free(wordArray[j]);
        }

        free(wordArray);
        return 0;

---

That is exactly how I have had it since you first mentioned the memory leak. If that is not the solution I don't know what is and at this point I couldn't care less.

Haha, must be getting tired. Missed that part.
You're right obviously. You outsmarted your teacher :D
However... scanf("&#37;s", ...) is unsafe. There's still one thing to fix ;)
And you can still do dynamic memory allocation (though it's a bit more advanced). So there's plenty more work to do if you want it.

It looks fine to me.

Also, I created a file with 500000 "words" [a, b, c .. z, aa, ab, ac... ba, bb, bc,... zz, etc]

the last ten are

Code:

---

qqkba
rqkba
sqkba
tqkba
uqkba
vqkba
wqkba
xqkba
yqkba
zqkba

---

I then instrumented the code to measure the time it takes to read the words in, and it toook about 0.05 seconds the first time round, and 0.16 s the second time round. So although I agree that it's inefficient to read the file twice, it's not as bad as it may seem.

This of course assumes you're not reading the data from a network, slow CDROM or some such - in that case, I'd recommend copying the file to some local storage first.

--
Mats

Quote:

---

Originally Posted by Elysia

Haha, must be getting tired. Missed that part.
You're right obviously. You outsmarted your teacher :D
However... scanf("%s", ...) is unsafe. There's still one thing to fix ;)
And you can still do dynamic memory allocation (though it's a bit more advanced). So there's plenty more work to do if you want it.

---

Thanks. I appreciate all of the advice. This was only the first part of the assignment, the real part is to set all this into a binary tree (I am using an AVL tree) and a hash table with options for the user to spell check and or delete words from a supplies .txt. Once I can finish the more difficult parts, if I still have time I will come back and fix the allocation and scanf.

So yes, it's an optional optimization the OP may do.
Also noticed fgets(wordString, 30, file) <--- it's better to do fgets(wordString, sizeof(wordString), file).
I don't know if I can think of anything else.

Quote:

---

Originally Posted by Elysia

So yes, it's an optional optimization the OP may do.
Also noticed fgets(wordString, 30, file) <--- it's better to do fgets(wordString, sizeof(wordString), file).
I don't know if I can think of anything else.

---

Ah! good idea!

Updated that as well.

Edit: hey I may have been a pain in the ass but at least I didn't post my assignment and ask someone to finish it all for me, yah? haha. Thansk again Mats and Elysia

Not a pain at all.

I expanded my test-input to 5M words, to make the time a bit more measurable. Using realloc and reading the file once takes 1.69s.

Reading the file twice, takes 0.61 + 1.67s. So there's a little bit of time to be gained by not reading it twice - but I think it's too small a gain to really worry about it.

--
Mats