Thread: 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;
}

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.

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

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];

??

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!

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).

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...

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);

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.

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
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.