Trying to extract strings from a text file and save them to a linked list

[blongnec]

I cannot seem to figure out how to get this to work. I need a few suggestions/help because the program keeps crashing and it does not insert the strings into the linked list properly.

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


struct listNode
{
  char *data;
  struct listNode *nextPtr;
};


typedef struct listNode LISTNODE;
typedef LISTNODE *LISTNODEPTR;


void insert(LISTNODEPTR *, char *);
void printList(LISTNODEPTR);


int main()
{
  LISTNODEPTR root = NULL;
  FILE *fptr;
  char file_name[20];
  char str[1000];
  int numOfChar;
  char *token;
  int count = 0;


  printf("Enter the name of the file: ");
  scanf("%s",file_name);


  printf("Enter the number of characters per line: ");
  scanf("%d",&numOfChar);


  fptr=fopen(file_name,"r");


  while(fgets(str, sizeof str, fptr) != NULL)
  {
      token = fscanf(fptr, "%s", str);
    while (token != NULL)
    {
        insert(&root, token);
        token = fscanf(fptr, "%s", str);
    }


  }


  fclose(fptr);


  printList(root);


  return 0;
}


void insert(LISTNODEPTR *sPtr, char *value)
{
   LISTNODEPTR newPtr, previousPtr, currentPtr;
   char *str;


   newPtr = malloc(sizeof(LISTNODE));
   str= (char *)malloc(60*sizeof(char));
   strcpy(str, value);


   if (newPtr != NULL) {/* is space available */
      newPtr->data = str;
      newPtr->nextPtr = NULL;


      previousPtr = NULL;
      currentPtr = *sPtr;


      if (previousPtr == NULL) {
         newPtr->nextPtr = *sPtr;
         *sPtr = newPtr;
      }
      else {
         previousPtr->nextPtr = newPtr;
         newPtr->nextPtr = currentPtr;
      }
   }
   else
      printf("%s not inserted. No memory available.\n", value);
}


void printList(LISTNODEPTR currentPtr)
{
   if (currentPtr == NULL)
      printf("List is empty.\n\n");
   else {


      while (currentPtr != NULL) {
         printf("%s -->", currentPtr->data);
         currentPtr = currentPtr->nextPtr;
      }


      printf("NULL\n\n");
   }
}

[laserlight]

First, two comments about conventions.

1. Generally, fully capitalised names are reserved for macro names, or for non-macro constants. Hence, this:

typedef struct listNode LISTNODE;

would be better as:

typedef struct listNode ListNode;

2. Unless you are dealing with an opaque pointer, it is generally a bad idea to typedef pointers as that can make them appear to be non-pointer types. So I would remove this typedef:

typedef LISTNODE *LISTNODEPTR;

Just use ListNode* when you need a pointer to ListNode.

3. When inserting into a linked list, there are three possible general scenarios: insert at the head, insert at the tail, insert after the given node. It looks like your insert function inserts at the head, so it would be good for its name to reflect that.

Therefore, this:

void insert(LISTNODEPTR *, char *);

would become:

void insertAtHead(ListNode **head, char *value);

Notice that besides renaming the function, I also included the parameter names in the forward declaration, and in fact used the more descriptive name head instead of sPtr. If you prefer, you could also have used root, though personally I tend to use that name when dealing more generally with trees rather than specifically with linked lists.

Now that this has been done, implementing insertAtHead becomes easier:

allocate space for the new node
if the space allocation is not successful
    return

allocate space for the new node's value
if the space allocation is not successful
    return

copy the input value to be the new node's value
set the next pointer of the new node to *head
set *head to the new node

Notice that this handles both the case where the list is empty and when it already has elements.

[blongnec]

Ok thanks that helped a lot. Do you know of a better/easier way to extract strings from a text file? My method just seems to crash.

[laserlight]

You probably just have bugs in the implementation. Also, it might make more sense to append at the tail instead of prepending at the head.

[algorism]

    token = fscanf(fptr, "%s", str);
    while (token != NULL)
    {
        insert(&root, token);
        token = fscanf(fptr, "%s", str);
    }

You're treating fscanf kind of like it's strtok. fscanf doesn't return a pointer to the string it's read. The string is returned in your str argument. fscanf returns the number of items read, so you're code should be:

    int nitems = fscanf(fptr, "%999s", str);
    while (nitems == 1)
    {
        insert(&root, str);
        nitems = fscanf(fptr, "%999s", str);
    }

Note that I also added a limit to the maximum number of characters read.