CODEWITHSUNDEEP
cfile-io
filpa
filpa

Reputation: 3644

Using fgets to copy lines into string array?

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.

Upvotes: 0

Views: 14894

Answers (2)

999k
999k

Reputation: 6565

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

Upvotes: 1

Jonathan Leffler
Jonathan Leffler

Reputation: 754050

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.

Upvotes: 3

Related Questions