Having trouble printing string from a file

Question

I have a task to read words from a file and print each word separately. So far I got this:

#define MAX 100000
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
typedef struct node *ptr;
typedef struct node {
    char *data;
    ptr next;
} node;

ptr head = NULL;

void add(char *data) {
        node *new_node = malloc(sizeof(node));
        new_node->data = data;
        new_node->next = head;
        head = new_node;
    printf("Adding new word:%s\n",data);
}

void print_list() {
    node *current = head;
        while (current != NULL) {
            printf("%s\n", current->data);
            current = current->next;
        }
}

void free_list(ptr *hnode)
{
    ptr p;
    while(*hnode){
        p = *hnode;
        *hnode = (*hnode)->next;
        free(p);
    }
}

int main(int argc, char *argv[]) {
    FILE *file;
    char buffer[MAX];
    char ch;
    int i = 0;

    if (argc != 2) {
        printf("Usage: %s <file>\n", argv[0]);
            exit(EXIT_FAILURE);
        }

        file = fopen(argv[1], "r");
        if (file == NULL) {
            printf("Error opening file\n");
            exit(EXIT_FAILURE);
        }

    while(i < MAX-1 && !feof(file))
    {
        if((ch = fgetc(file)) == ' '|| ch == '\n')
        {
            buffer[i] = '\0';
            add(buffer);
            i = 0;
        }
        else{
            if(ch == EOF)
                  break;
            buffer[i++] = ch;
        }
    }

        print_list();
        free_list(&head);
        exit(EXIT_SUCCESS);
}

When I try to run the code there are no compilations or runtime errors but I get only the last word and a weird symbol, for example:

Adding new word:Hello
Adding new word:World!
Adding new word:Does
Adding new word:my
Adding new word:code
Adding new word:work?
work?
work?
work?
work?
work?
work?

The part when it says adding new word is correct and reads the file properly, my problem is the printing part. Edit: Now I have a proper EOF check, still doesn't work as intended.

You are mistaken believing that _The part when it says adding new word is correct_. — Armali, Jun 03 '22 at 13:33
`ch` must be `int` ... you need to check for `EOF` ... [`while(feof(file))` is always wrong](https://stackoverflow.com/questions/5431941/why-is-while-feof-file-always-wrong) — pmg, Jun 03 '22 at 13:33
Please do not turn to Stack Overflow to debug your whole program. You should analyze the individual parts of it and monitor its execution step-by-step if necessary to find an individual problem. At that point, you can generally fix the individual problem yourself. If there is some concept you do not quite understand, you should first use a textbook and other documentation to learn about it, then ask your teacher or teaching assistant. Only after that might it be appropriate to ask Stack Overflow. There are multiple errors in your program. — Eric Postpischil, Jun 03 '22 at 13:37
"Now I have a proper EOF check, still doesn't work as intended" -- You still do not have a "proper EOF check". You are still attempting to store `EOF` in a `char`, although it is not guaranteed to be able to represent the value `EOF`. Note that the return value of `fgetc` is of type `int`, not `char`. You need an `int` in order to be sure that the variable is able to represent the value `EOF`. Even on platforms on which a `char` is able to represent the value `EOF`, you will not be able to distinguish the value `EOF` from an actual character code, if you truncate the value to `char`. — Andreas Wenzel, Jun 03 '22 at 13:48

Daniel Walker · Accepted Answer · 2022-06-03T13:55:23.843

When you call add(buffer), you're passing the exact same pointer every time (technically, arrays aren't pointers but it's close enough for this discussion). That pointer gets assigned to the data field of your nodes. This means that every node contains a reference to the exact same string. That's why every node is printing out the same value. In addition, as your code becomes more complex, you run the risk of these references becoming invalid if buffer's lifetime ever expires.

What you want to do is use malloc to give each node its own copy of the string.

void add(char *data) {
    size_t len = strlen(data);
    node *new_node = malloc(sizeof(node)); // Note: You should check this return value.
    new_node->data = malloc(len+1); // Note: Ditto.
    memcpy(new_node->data, data, len+1);
    new_node->next = head;
    head = new_node;
    printf("Adding new word:%s\n",data);
}

void free_list(ptr *hnode)
{
    ptr p;
    while(*hnode){
        p = *hnode;
        *hnode = (*hnode)->next;
        free(p->data);
        free(p);
    }
}

Vlad from Moscow · Answer 2 · 2022-06-03T13:55:20.487

In the function add data members data of all nodes point to the same array buffer declared in main

void add(char *data) {
        node *new_node = malloc(sizeof(node));
        new_node->data = data;
        new_node->next = head;
        head = new_node;
    printf("Adding new word:%s\n",data);
}

So the function print_list will output what is last stored in the buffer.

You need to make dynamically a copy of the passed string.

Also this while loop

while(i < MAX-1 && !feof(file))
{
    if((ch = fgetc(file)) == ' '|| ch == '\n')
    {
        buffer[i] = '\0';
        add(buffer);
        i = 0;
    }
    else{
        buffer[i++] = ch;
    }
}

does not make a great sense. First of all the variable ch shall be declared as having the type int. The condition in the loop

while(i < MAX-1 && !feof(file))

can result in storing the value EOF in the buffer. And the output demonstrates this.

(Edit: this update in your question

       if(ch == EOF)
              break;

you made after my answer.)

Also reading strings by one character is inefficient. You should use the function fscanf instead of fgetc.

Pay attention to that it is a bad idea to define the global variable head

ptr head = NULL;

and when some functions depend on the global variable.

Also in some parts of the program you are using the typedef name ptr while in other parts of the program you are using the type node *. This only confuses readers of the code.

Having trouble printing string from a file

2 Answers2