0

i am trying to read lines from a text file into char array, but there is something wrong. Please see the code and let me know what am i doing wrong. thanks.

#include <stdio.h>
#include <stdlib.h>
int main(void) {
    int i=0,j;
    char* string[100];
    char line[100];
    FILE *file; 
    file = fopen("patt", "r"); 
    while(fgets(line, sizeof line, file)!=NULL) {
        printf("%d %s",i, line);
        string[i]=line;
        i++;
    }
    for (j=0 ; j<i ; j++) {
        printf("string[%d] %s",j, string[j]);
    }
    fclose(file);
    return 0;
}

the input file patt has the following contents.

rec
cent
ece
ce
recent
nt

On executing the codes above i get this

0 rec
1 cent
2 ece
3 ce
4 recent
5 nt
string[0] nt
string[1] nt
string[2] nt
string[3] nt
string[4] nt
string[5] nt

what i expect is this

0 rec
1 cent
2 ece
3 ce
4 recent
5 nt
string[0] rec
string[1] cent
string[2] ece
string[3] ce
string[4] recent
string[5] nt
rkrara
  • 442
  • 1
  • 6
  • 17
  • Asking people to spot errors in your code is not productive. You should use the debugger (or add print statements) to isolate the problem, and then construct a [minimal test-case](http://sscce.org). – Oliver Charlesworth Jan 20 '13 at 17:12

3 Answers3

3

You are repeatedly writing to the same array.

while(fgets(line, sizeof line, file)!=NULL) {

In this line you repeatedly, write to the array line.


I think you are misunderstanding what char* string[100]; does. You are using it as an array of arrays, while it actually is an array of 100 pointers.

To correct the problem, you need to first allocate a new memory block in every iteration :

 string[i] = malloc(strlen(line)+1);

Then you need to copy the contents of line to string[i] :

strcpy(string[i], line); 

Also, at the end of the program you will need to use free to release the memory.

asheeshr
  • 4,088
  • 6
  • 31
  • 50
  • what is correct way to assign those lines read from file to the string array – rkrara Jan 20 '13 at 17:13
  • thanks a lot, it works now PS: could you suggest better way to this char* string[100]; char line[100]; – rkrara Jan 20 '13 at 17:19
  • 1) Don't cast the return from `malloc` (doing so can hide a bug). 2) You haven't allocated enough space for the NUL terminator on the string -- you need ` malloc(strlen(line) + 1)`. – Jerry Coffin Jan 20 '13 at 17:20
  • @JerryCoffin Whats the reasoning for not casting the pointer returned from malloc ? I have seen a few posts on SO saying the same, but no explanation. – asheeshr Jan 20 '13 at 17:21
  • @AshRj: the cast will stop the compiler for complaining even though you haven't included the proper prototype for `malloc` with `#include ` -- but without the prototype, you can get incorrect behavior (the compiler assumes it returns an `int`, then casts that to a pointer, rather than knowing it returns a pointer). If/when `int` and `void *` are different sizes (e.g., 64-bit code on some compilers) this won't work correctly. – Jerry Coffin Jan 20 '13 at 17:36
  • 1
    @AshRj You should have stumbled over [my answer here](http://stackoverflow.com/a/605858/28169) which really does try to provide some explanation. – unwind Jan 20 '13 at 17:38
2

Your string[i]=line; means you're repeatedly storing a pointer to the same buffer (line) into each successive item in string. Something like this:

enter image description here

To make things work, you need to allocate a new buffer for each of those to point at, something like: string[i] = dupe_string(line);, where dupe_string will be something like:

char *dupe_string(char const *in) { 
    char *dupe;
    if (NULL != (dupe = malloc(strlen(in) + 1)))
        strcpy(dupe, in);
    return dupe;
}

Note that since this uses malloc to allocate space for each string, you'll eventually need to call free for each of those to avoid having a memory leak.

Jerry Coffin
  • 476,176
  • 80
  • 629
  • 1,111
  • thanks, do you mean I have to use something like for(i=0 to 100) and then free(string[i]) – rkrara Jan 20 '13 at 17:23
  • i tried to implement this on what Ashrj suggested but there are errors like these -> test.c:23:8: error: conflicting types for ‘dupe_string’ test.c:12:16: note: previous implicit declaration of ‘dupe_string’ was here test.c: In function ‘dupe_string’: test.c:26:11: error: ‘dup’ undeclared (first use in this function) test.c:26:11: note: each undeclared identifier is reported only once for each function it appears in – rkrara Jan 20 '13 at 17:29
  • @user1733911: Yes, except that you want to ensure you only free as many strings as you allocated (calling `free` on whatever random value an uninitialized pointer might contain will give undefined behavior). Alternatively, you can initialize the un-used pointers to `NULL`. As for the others: I had a typo in the code --`dup` should have been `dupe`. You can either put `dupe_string` *before* the code that uses it, or else add a prototype like `char *dupe_string(char const *);` before the function that calls it. – Jerry Coffin Jan 20 '13 at 17:29
1

To help you out understanding your code:

  • You are writing to string[i] the address of the line every time, while the content of the array line keeps changing throughout the loop. This results in, all string[i] to contain the address of the same variable i.e. line. At the end of the loop, the last line read from the file is stored in line array. Now while you are printing string[i], you are printing the data present in the variable line.Since, all the instance of string[i] contain the same address of line, they all are printing the same value i.e. nt in your output.
int main(void) {
    int i=0,j;
    char* string[100];
    char line[100];
    FILE *file; 
    file = fopen("patt", "r"); 
    while(fgets(line, sizeof(line), file)!=NULL) {//You missed the ()'s in the sizeof
        printf("%d %s",i, line);
        string[i]=line;
        i++;
    }

*What you should have instead done is:

int main(void) {
int i=0,j;
char* string[100];
char line[100];
FILE *file; 
file = fopen("patt", "r"); 
while(fgets(line, sizeof(line), file)!=NULL) {//You missed the ()'s in the sizeof
    printf("%d %s",i, line);
    string[i]=malloc(strlen(line)+1); ///You need to allocate some memory to char* string[i] here
    if(string[i] == NULL)//Do a NULL check if malloc failed
      //Handle NULL here
    strncpy(string[i],line,strlen(line)+1);//Use strncpy to copy line into the malloced memory in string[i]
    i++;
}

Now, since you have malloced the memory for storing you data you need to free the memory too. So, instead of this in your code:

    for (j=0 ; j<i ; j++) {
        printf("string[%d] %s",j, string[j]);
    }
    fclose(file);
    return 0;
}

Do this:

    for (j=0 ; j<i ; j++) {
    printf("string[%d] %s",j, string[j]);
    free(string[j]);
    }
   fclose(file);
   return 0;
}

Now, that should give you what you wanted.

askmish
  • 6,464
  • 23
  • 42
  • Your code doesn't include NUL terminators when it creates the duplicate strings, so when he tries to print them out, he'll get undefined behavior. – Jerry Coffin Jan 20 '13 at 17:33
  • yes i got hell lot of crazy errors – rkrara Jan 20 '13 at 17:39
  • It was a typo and so was `free(string[i]);`. – askmish Jan 20 '13 at 17:41
  • The code is fixed now. Note that you can store max. 100 lines due to `char* string[100];` in your code. You can use `realloc()` instead of using a static array for reading as many lines as needed, from the file. – askmish Jan 20 '13 at 17:47
  • @askmish: I think "fixed" it a bit strong of a word. You've managed to make the zigs and zags match up correctly so you no longer have undefined behavior, but code like: `strncpy(string[i],line,strlen(line)+1);` is...horrible. In fact, anytime you even consider using `strncpy`, stop, take a deep breath, and think. If `strncpy` is the right answer, you're almost certainly asking the wrong question. – Jerry Coffin Jan 20 '13 at 18:13