Scanning in a text file into a linked list

Question

I'm just learning about linked lists and I have to do an assignment that has many parts, but I'm starting out and the very first thing I need to do is read in an input file into a linked list. Part of the file is:

George Washington, 2345678 John Adams, 3456789 Thomas Jefferson, 4567890 James Madison, 0987654 James Monroe, 9876543 John Quincy Adams, 8765432

and contains a total of 26 lines.

All I want to do now is simply read in the file. I try by using this code (in main for now)

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


struct node{

    char name[20];
    int id;
    struct node *next;

}*head;



int main(void){

    struct node *temp;
    temp = (struct node *)malloc(sizeof(struct node));
    head = temp;

    FILE *ifp;
    ifp = fopen("AssignmentOneInput.txt", "r");

    int c = 0;

    while(c<26){
        fscanf(ifp, "%s", &temp->name);
        fscanf(ifp, "%d", &temp->id);
        printf("%d\n", c);
        temp = temp->next;
        c++;
    }

For the output, I know that the first name and the first ID are scanned in, because the value of c is displayed as 0 (right now I'm arbitrarily using the value of c to control the fscanf). But after that, the program crashes. So the problem must be with temp = temp->next; It compiles fine.

I am very new to linked lists, so I really don't know what I'm doing.

Your help is appreciated!

Answer 1

Primary issue is certainly temp = temp->next sets temp to field next that was never initialized, causing code to seg. fault on the next loop.

There are linked list issues and input issues. Recommend do not allocate space until good data is found.

Start with a temp_head. Code only uses the next field of temp_head.

struct node temp_head;
temp_head.next = NULL;
struct node *p = &temp_head;

Whenever code reads line data, recommend using fgets() to read the line and then scan the buffer.

char buf[100];
while (fgets(buf, sizeof buf, ifp) != NULL) {
  struct node nbuf;

Scan the buffer using sscanf(). Use '%[^,]' to read until ','.

  if (2 != sscanf(buf, " %19[^,],%d", nbuf.name, &nbuf.id)) {
    break;  // Invalid data encountered
  }
  nbuf.next = NULL;

  // Code does not allocate data until good data was found 
  p->next = malloc(sizeof *(p->next));
  if (p->next == NULL) break;  // OOM
  p = p->next;
  *p = nbuf;  // Copy the data
}

head = temp_head.next;

---

Notes:

The cast in temp = (struct node *)malloc(sizeof(struct node)); is not needed.

Consider this style of allocation: temp = malloc(sizeof *temp), IMO it is easier to code and less to maintain.

fscanf(ifp, "%s", &temp->name); fscanf(ifp, "%d", &temp->id); has 3 problems: no limitation on string input, unneeded & and failure to check scan results. Notice above code uses (2 != sscanf(buf, " %19[^,], %d", nbuf.name, &nbuf.id), which limits string input to 19 char (leaving room for the terminating '\0', does not use a & when the field is an array, and checks that 2 fields were successfully scanned.

Before the end of main(), code should free the data allocated.

Answer 2

First of all, since you are writing in C, there's no need to cast the malloc.

Second, you must allocate memory for each new node yourself.

Third, the name of an array already decays to a pointer, so you should not take & of it because then you'll get a pointer to a pointer which is not what you want.

Finally,you need to fix your scanf syntax to handle spaces in your fields.

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

#include <string.h>

struct node{

    char name[20];
    int id;
    struct node *next;

}*head;



int main(void){

    struct node *temp;
    temp = malloc(sizeof(struct node));
    temp->next = NULL;
    head = temp;

    FILE *ifp;
    ifp = fopen("AssignmentOneInput.txt", "r");

    int c = 0;

    char buffer[1024];
    memset(buffer, 0, 1024);
    while(c<5){
        fgets(buffer, 1024, ifp);
        sscanf(buffer, "%19[^,], %d", temp->name, &temp->id);
        printf("%d %s %d\n",c, temp->name, temp->id);
        temp->next = malloc(sizeof(struct node));
        temp = temp->next;
        temp->next = NULL;
        c++;
    }
}

Answer 3

In the following lines you've allocated enough space for a single element of your list (one struct node), and pointed your head pointer to it:

temp = (struct node *)malloc(sizeof(struct node));
head = temp;

Later you read in values into name and id fields of this element:

fscanf(ifp, "%s", &temp->name);
fscanf(ifp, "%d", &temp->id);

But what does temp->next point to? You've only thus far allocated space for a single element. You need to allocate space for each subsequent element as you add it to the list.

Edit: As @merlin2011 indicated below, this answer will simply help you resolve the program crash, but won't completely get your program working as you might expect. However, hopefully you'll be able to better debug it once it's not crashing.