Using fgets to copy lines into string array?

Question

I've got a C program that I'm trying to write which intends to reverse the lines of a file. I am still very inept at C (though I come from a Java background), so I am very likely making mistakes with pointers and such, but I've tried consulting the manual at every turn. This is somewhat of an assignment.

The point of the program is to reverse a file's contents, up to MAX_LINES in number, each line being no longer than MAX_CHARS. The method I wanted to try is the following: use fgets to read from the file, 80 chars or until EOL, store that string, and repeat the process until either EOF or MAX_LINES has been reached, using a counter on the side. Afterwards I simply put the same string in a different array, going from second_array[counter] to 0. However, I am having issues with actually getting the strings into the first array. Here's what I have so far:

1 #include <stdio.h>
2 #include <string.h>
3
4 #define MAX_LINES 100
5 #define MAX_CHAR 80
6
7 int main(int argc, char *argv[])
8 {
9         if(argc != 2)
10                 goto out;
11         FILE *fp;
12         char *str[MAX_CHAR];
13         char buffer[MAX_CHAR];
14         char *revstr[MAX_CHAR];
15         int curr_line = 0, i = 0, j =0;
16
17         fp = fopen(argv[1],"r");
18
19         out:
20         if (fp == NULL && argc != 2){
21                 printf("File cannot be found or read failed.\n");
22                 return -1;
23         }
24
25         /* printf("This part of the program reverses the input text file, up to a  maximum of 100 lines and 80 characters per line.\n The reversed file, from the last (or    100th) line to the first, is the following:\n\n"); */
26
27         /* fgets reads one line at a time, until 80 chars, EOL or EOF */
28         while(curr_line < MAX_LINES && ((fgets(buffer, MAX_CHAR, fp)) != NULL)){
29
30                 str[i] = buffer;
31                 ++j;
32                 ++i;
33         }
34
35         for(i = 0; i < 4; ++i) 
36                 printf("%s \n", str[i]);
37
38
39         printf("END OF PROGRAM RUN.");
40         return 0;
41 }

In the same directory, I have a "txt" file which contains the following lines:

is this a test
this is a test
this is not a test

When I compile and run the program however (./a.out txt), I get the following output:

this is not a test

this is not a test

this is not a test

END OF PROGRAM RUN.

Obviously this means that is is overwriting the same location, but I am not sure how to rectify this (as said, pointers are still pretty alien to me). Could anyone clarify what is happening here? Do I need to use a 2D array instead? Any help would be greatly appreciated.

Jonathan Leffler · Accepted Answer · 2013-01-22 06:09:17Z

3

Misuse of `buffer`

The trouble is that you keep storing the pointer buffer in str[i], but buffer only contains the last line read.

You will have to make copies of the lines read, maybe using strdup():

str[i++] = strdup(buffer);

There's error checking to worry about, but conceptually, that's one simple way to deal with it.

Could you please clarify as to why that is? The declaration of fgets() being char *fgets(), I got the impression that it momentarily returns a pointer to the line currently read from the file!

Actually, fgets() returns one of two values:

NULL on EOF or other I/O error, or
buffer, the pointer to the string passed to the function.

In particular, it does not create new storage for you automatically. If you want that, you probably need to look at readline() from POSIX.

So, your calls to fgets() use the same array, buffer, each time, so that each successive line read overwrites the previous one. That's why you need to copy each line somehow, and why I suggested strdup() as an easy way to do that.

strdup()

The function strdup() is part of POSIX but not standard C. However, it is easily implemented:

char *strdup(const char *str)
{
    size_t len = strlen(str) + 1;
    char  *dup = malloc(len);
    if (dup != 0)
        memmove(dup, str, len);
    return(dup);
}

Error reporting

You should avoid goto when possible, which is easily done with this code. Your dual-purpose error message actually ends up confusing the user who omits the argument, or supplies too many arguments. Provide two separate error messages, one for the incorrect number of arguments to the program, and one for failing to open the file.

if (argc != 2)
{
    fprintf(stderr, "Usage: %s file\n", argv[0]);
    exit(1);
}
fp = fopen(argv[1], "r");
if (fp == 0)
{
    fprintf(stderr, "%s: failed to open file %s (%d: %s)\n",
            argv[0], argv[1], errno, strerror(errno));
    exit(1);
}

Note that errors should be reported on stderr, not stdout; that's what the error output channel is for.

edited Jan 22, 2013 at 6:09

answered Jan 22, 2013 at 5:54

Jonathan Leffler

759k145 gold badges961 silver badges1.3k bronze badges

Sign up to request clarification or add additional context in comments.

2 Comments

filpa Over a year ago

The trouble is that you keep storing the pointer buffer in str[i], but buffer only contains the last line read. Could you please clarify as to why that is? fgets' declaration being char *fgets, I got the impression that it momentarily returns a pointer to the line currently read from the file!

filpa Over a year ago

Your answer explains excellently what was wrong with my code, and I see you even edited it to answer my comment. Your error reporting code of conduct note was also very helpful. Thank you very much!

999k · Accepted Answer · 2013-01-22 06:24:47Z

1

In you code you are making each index of str array to point to same array buffer. Thats is why you are getting same value (value in buffer) for evey index in str array. You can use code using array like this

#define MAX_LINES 100
#define MAX_CHAR 80

int main(int argc, char *argv[])
{
        if(argc != 2)
                 goto out;
         FILE *fp;
         char str[MAX_LINES][MAX_CHAR];
         char buffer[MAX_CHAR];
         char *revstr[MAX_CHAR];
         int curr_line = 0, i = 0, j =0;

         fp = fopen(argv[1],"r");

         out:
         if (fp == NULL && argc != 2){
                 printf("File cannot be found or read failed.\n");
                 return -1;
         }

         /* printf("This part of the program reverses the input text file, up to a  maximum of 100 lines and 80 characters per line.\n The reversed file, from the last (or    100th) line to the first, is the following:\n\n"); */

         /* fgets reads one line at a time, until 80 chars, EOL or EOF */
         while(curr_line < MAX_LINES && ((fgets(buffer, MAX_CHAR, fp)) != NULL)){

                 //str[i] = buffer;
                 memcpy(str[i], buffer, strlen(buffer));
                 curr_line++;
                 ++j;
                 ++i;
         }

         for(i = 0; i < 4; ++i) 
                 printf("%s \n", str[i]);


         printf("END OF PROGRAM RUN.");
         return 0;
 }

Also you are not modifying variable currn_line

answered Jan 22, 2013 at 6:24

999k

6,6452 gold badges31 silver badges32 bronze badges

1 Comment

filpa Over a year ago

Thank you very much! I wish I could select multiple answers, as memcpy seems to be a very simple way to get the desired effect. I did notice the curr_line variable, but thank you for pointing that out. A few other vars, as pointed out, were not used because I got stuck partway through and wanted to debug first: but I ended up not needing some either way.

Collectives™ on Stack Overflow

Using fgets to copy lines into string array?

2 Answers 2

Misuse of `buffer`

strdup()

Error reporting

2 Comments

1 Comment

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

Misuse of buffer

strdup()

Error reporting

2 Comments

1 Comment

Your Answer

Sign up or log in

Post as a guest

Linked

Related

Misuse of `buffer`