adding contents from file to linked lists

Question

I am pretty new in C Programming.

I want to add contents from a file to a link list.

The contents from the file are names and each name should have their own node.

However when run the code I get an infinite loop. I have tried to solve but I just can't get to the bottom of it. I have a feeling the fscanf is causing the problem.

Thank you in advance.

#include <stdio.h>
#include <stdlib.h> 
#include <string.h>

  typedef struct node {
    char name[50];
    struct node * next;
  }
node;

int main() {

  FILE * file = fopen("content.txt", "r");
  if (file == NULL)
    return 1;

  char buf[50];

  node * first = NULL;

  //read the contents of the file and write to linked list
  while (!feof(file)) {
    fscanf(file, "%s", buf);

    // try to instantiate node
    node * head = malloc(sizeof(node));
    if (head == NULL) {
      return -1;
    }

    head-> next = NULL;

    // add new word to linked list
    strcpy(head-> name, buf);

    //move node to start of linked list
    head-> next = first;
    first = head;

  }

  fclose(file);
  node * ptr = first;
  while (ptr != NULL) {
    printf("%s\n", ptr-> name);
  }

  return 0;

}

This is how the input file looks like.

REDA
REDB
REDC
REDE
RED 
REDb
REDc
REDpikachu
REDboom

Answer 1

I think the problem is the output part.

  while (ptr != NULL) {
    printf("%s\n", ptr-> name); // here is the infinite loop
    ptr = ptr->next; // add this 
  }

you can fix it by adding a line to update the value of ptr as shown above.

Answer 2

#include <stdio.h>
#include <stdlib.h> 
#include <string.h>

typedef struct node {
    char name[50];
    struct node * next;
} node;

void list_insert(node **head,char *data)
{
    node *new;

    new = malloc(sizeof(node));
    new->next = *head;
    strcpy(new->name,data);
    *head = new;
}

int main() {
    char buf[50];
    node * first = NULL;

    FILE * file = fopen("content.txt", "r");
    if (file == NULL) {
        return 1;
    }
    while (!feof(file)) {
      fscanf(file, "%s", buf);
      list_insert(&first,buf);
    }
    fclose(file);

    node * ptr = first;
    while (ptr != NULL) {
        printf("%s\n", ptr-> name);
        ptr = ptr->next;
    }
    return 0;
}

Answer 3

You have a number of issues that need to be corrected in your code (some more nits than others).

Your first problem is can be solved by simply reading Why is while ( !feof (file) ) always wrong?. (the short answer is EOF is not generated on the final successful read by fscanf, so after reading the last "name" you check !feof(file) which tests TRUE, the loop continues, fscanf fails with EOF, you allocate and then attempt to strcpy from buf holding an indeterminate value -- invoking Undefined Behavior)

While in this case, where reading words without whitespace, you can use fscanf -- but don't. Form good habits early. When taking input from each line of a file, use a line-oriented input function (e.g. fgets or POSIX getline) to insure what is left in your input buffer doesn't depend on the scanf conversion specifier used.

For example, you could:

#define MAXN 50     /* if you need a constant, #define one (or more) */

typedef struct node {
    char name[MAXN];
    struct node *next;
}
node;

... /* read the contents of the file and write to linked list */ while (fgets (buf, MAXN, file)) {

        /* valdiating the last char is '\n' or length < MAXN - 1 omitted */

        /* allocate new node */
        node *head = malloc (sizeof(node));
        if (head == NULL) {         /* validate allocation */
            perror ("malloc-node"); /* issue error message */
            return 1;   /* don't return negative values to the shell */
        }

        /* trim '\n' from end of buf */
        buf[strcspn(buf, "\n")] = 0;    /* overwrite with nul-character */

        /* initialize node values */
        strcpy (head->name, buf);
        head->next = NULL;

(note: don't return negative values to your shell, and do NOT include a space after the -> operator when referencing members of a struct)

At this point, you have successfully read "name" into buf, allocated for your new node, validated the allocation succeeded, trimmed the trailing '\n' included in buf by the line-oriented input function, copied the trimmed "name" to head->name and initialized head->next to NULL. However, you have a problem. All your names are stored in your list in reverse-order (which at times may be what you want). But this isn't normally the desired behavior. By simply declaring a single additional pointer that is updated to point to the last node, you can do an in-order insertion into the list without iterating to find the last node.

For example, simply declaring a last pointer and checking two cases (1) last=NULL indicating the loop is empty and making first and last point to the first node allows you to (2) add all other nodes at the end, e.g.:

    node *first = NULL, *last = NULL;   /* use last for in-order chaining */
    ...
        /* initialize node values */
        strcpy (head->name, buf);
        head->next = NULL;

        /* in-order add at end of list */
        if (!last)
            first = last = head;
        else {
            last->next = head;
            last = head;
        }
    }

Your "Infinite Loop" was correctly addressed by @CS Pei where you simply forgot to advance the current pointer to ptr->next in your output loop.

You have additionally failed to free the memory you have allocated. Yes, it is freed on program exit, but get in the habit of tracking all memory you allocate and then freeing that memory when it is no longer needed. (that become a vital skill to preventing memory leaks as your code grows). For example, in your output loop, you could do:

    node *ptr = first;              /* declare/set pointer to first */
    while (ptr != NULL) {           /* loop while ptr */
        node *victim = ptr;         /* declare/set pointer to victim */
        printf ("%s\n", ptr->name); /* output name */
        ptr = ptr->next;            /* advance pointer to next */
        free (victim);              /* free victim */
    }

Putting it altogether, you could do something similar to the following:

#include <stdio.h>
#include <stdlib.h> 
#include <string.h>

#define MAXN 50     /* if you need a constant, #define one (or more) */

typedef struct node {
    char name[MAXN];
    struct node *next;
}
node;

int main (int argc, char **argv) {

    FILE *file = argc > 1 ? fopen (argv[1], "r") : stdin;
    if (file == NULL)
        return 1;

    char buf[MAXN];

    node *first = NULL, *last = NULL;   /* use last for in-order chaining */

    /* read the contents of the file and write to linked list */
    while (fgets (buf, MAXN, file)) {

        /* valdiating the last char is '\n' or length < MAXN - 1 omitted */

        /* allocate new node */
        node *head = malloc (sizeof(node));
        if (head == NULL) {         /* validate allocation */
            perror ("malloc-node"); /* issue error message */
            return 1;   /* don't return negative values to the shell */
        }

        /* trim '\n' from end of buf */
        buf[strcspn(buf, "\n")] = 0;    /* overwrite with nul-character */

        /* initialize node values */
        strcpy (head->name, buf);
        head->next = NULL;

        /* in-order add at end of list */
        if (!last)
            first = last = head;
        else {
            last->next = head;
            last = head;
        }
    }

    if (file != stdin)  /* close file if not stdin */
        fclose(file);

    node *ptr = first;              /* declare/set pointer to first */
    while (ptr != NULL) {           /* loop while ptr */
        node *victim = ptr;         /* declare/set pointer to victim */
        printf ("%s\n", ptr->name); /* output name */
        ptr = ptr->next;            /* advance pointer to next */
        free (victim);              /* free victim */
    }

    return 0;
}

Example Use/Output

Using your input file, you would obtain the following output:

$ ./bin/ll_chain_last <dat/llnames.txt
REDA
REDB
REDC
REDE
RED
REDb
REDc
REDpikachu
REDboom

Memory Use/Error Check

In any code you write that dynamically allocates memory, you have 2 responsibilities regarding any block of memory allocated: (1) always preserve a pointer to the starting address for the block of memory so, (2) it can be freed when it is no longer needed.

It is imperative that you use a memory error checking program to insure you do not attempt to access memory or write beyond/outside the bounds of your allocated block, attempt to read or base a conditional jump on an uninitialized value, and finally, to confirm that you free all the memory you have allocated.

For Linux valgrind is the normal choice. There are similar memory checkers for every platform. They are all simple to use, just run your program through it.

$ valgrind ./bin/ll_chain_last <dat/llnames.txt
==24202== Memcheck, a memory error detector
==24202== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==24202== Using Valgrind-3.12.0 and LibVEX; rerun with -h for copyright info
==24202== Command: ./bin/ll_chain_last
==24202==
REDA
REDB
REDC
REDE
RED
REDb
REDc
REDpikachu
REDboom
==24202==
==24202== HEAP SUMMARY:
==24202==     in use at exit: 0 bytes in 0 blocks
==24202==   total heap usage: 9 allocs, 9 frees, 576 bytes allocated
==24202==
==24202== All heap blocks were freed -- no leaks are possible
==24202==
==24202== For counts of detected and suppressed errors, rerun with: -v
==24202== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

Always confirm that you have freed all memory you have allocated and that there are no memory errors.

Look things over and let me know if you have further questions.