Reading File Data Into a Linked List in C

Question

I am trying to create a simple phonebook program that reads data from a file and stores the content into specific nodes in the list. It works fine if I use my addEntry function with static data, such as:

addEntry("First", "Last", "555-555-5555");

If I try to read more than 1 entry from the file, each entry just appears to be whatever the last entry was in the file. For example, if my file contained:

First1
Last1
123-456-7890
First2
Last2
987-654-3210

After storing the data in the list and printing, the output would look like:

First2
Last2
987-654-3210

First2
Last2
987-654-3210

Rather than printing each specific name and number. This confuses me because this issue only occurs when I read data from the file, not when I manually type in the name and number in a function call. Here are the definitions for main and addEntry, thank you in advance.

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

struct bookNode
{
    char * firstName;
    char * lastName;
    char * phoneNumber;
    struct bookNode * next;
} * head;

FILE * fpointer;

void addEntry(char * fn, char * ln, char * pn);
void display();
int numEntries();
void writeBookData(struct bookNode * selection);

int main()
{
    head = NULL;
    addEntry("Test", "Name", "111-111-1111");
    addEntry("Test2", "Name2", "222-222-2222"); // These entries will work as intended

    int i;
    fpointer = fopen("addressbook.dat", "a+");
    if(fpointer == NULL)
    {
        printf("Error: addressbook.dat could not be opened.\n");
    }

    char first[20];
    char last[20];
    char num[20];

    while (!feof(fpointer))
    {
        fgets(first, 20, fpointer);
        fgets(last, 20, fpointer);
        fgets(num, 20, fpointer);

        //Removes newline characters from the ends of the names
        i = 0;
        while(first[i] != '\n')
        {
            i++;
        }
        first[i] = '\0';
        i = 0;
        while(last[i] != '\n')
        {
             i++;
        }
        last[i] = '\0';

        // Adds the entry from the strings with the file data in them
        addEntry(first, last, num);
}
fclose(fpointer);

display(); // typical linked list display function

int entryCount = numEntries();
printf("There are %d entries in this Address Book\n", entryCount);

return EXIT_SUCCESS;
}

void addEntry(char * fn, char * ln, char * pn)
{
struct bookNode * tempNode, * iterator;
tempNode = (struct bookNode *)malloc(sizeof(struct bookNode));
tempNode->firstName = fn;
tempNode->lastName = ln;
tempNode->phoneNumber = pn;
iterator = head;

// If the list is empty
if (head == NULL)
{
    head = tempNode;
    head->next = NULL;
}

// The list is not empty
else
{
    while(iterator->next != NULL)
    {
        iterator = iterator->next;
    }
    tempNode->next = NULL;
    iterator->next = tempNode;
}

}

Answer 1

You need to copy the string values to each new node. You are only storing the pointer to each string, but it is always the same pointers (first, last and num that are declared in main) so they all point to the same memory.

So in your addEntry method, you need to first allocate memory to store the string and then copy the string to the new memory.

Your example where you add the entries manually works because the char pointers point to static strings.

So in your addEntry method you should do something like this:

tempNode = (struct bookNode *)malloc(sizeof(struct bookNode));
tempNode->firstName = (char *)malloc(strlen(fn)+1);
strcpy(tempNode->firstName, fn);

and then the same for last name and phone. Remember that you need to go through the list and free the memory for each string as well as for the nodes in the list as well.

Answer 2

Your bookNode struct contains pointers to memory. Your addEntry function places a copy of these pointers into the list, but the memory they point to is still owned by the caller: it is, in fact, the first, last and num arrays you declare in main, that you then proceed to overwrite on the next iteration of the loop.

What your addEntry function needs to do instead of copying its input pointers is to allocate enough memory for the strings, copy the inputs into that memory and retain the pointers to the copies. You will also need to make sure you free all the memory you have allocated once you are done with it.

Answer 3

There are a couple of problems with your program.

What you are doing is equivalent of:

char data[SIZE];
char *p;
/* get some useful value in data */
p = data;

In the context of the last line, data refers to a pointer that points to the first element of the array data (i.e., the line is equivalent to p = &data[0];). So, what you just did was to assign to the pointer p the value of the address of the first character in data. Later, when you change the contents of data, the pointer to the first element of data is still the same (data still exists at the same memory location). So, all your pointers refer to the same storage, and you keep overwriting what's in the storage.

But then why does your program work when you provide literal strings? Because each literal string in C is guaranteed to exist throughout the life of the program and has a unique address. (There is a minor exception: if you use a literal string more than once in your program, it may or may not refer to the same memory.)

So, you should dynamically allocate memory for firstName, lastName, and phoneNumber members of your nodes, and remember to free them when you are done with it.

void addEntry(char *fn, char *ln, char *pn)
{
    struct bookNode *tempNode, *iterator;

    tempNode = malloc(sizeof *tempNode);
    tempNode->firstName = malloc(strlen(fn) + 1); /* +1 for terminating 0 */
    tempNode->lastName = malloc(strlen(ln) + 1);
    tempNode->phoneNumber = malloc(strlen(pn) + 1);

    /* Omitted check for malloc failures for brevity */
    strcpy(tempNode->firstName, fn);
    strcpy(tempNode->lastName, ln);
    strcpy(tempNode->phoneNumber, pn);

    /* Now continue with what you were doing */
}

Then, you will need a corresponding freeEntry function to free up the space.

Another way to do this would be to declare your struct differently:

#define MAX 20
struct bookNode
{
    char firstName[MAX];
    char lastName[MAX];
    char phoneNumber[MAX];
    struct bookNode *next;
} *head;

Then, your addEntry function doesn't need the malloc() calls for firstName, lastName, and phoneNumber, but you still need to copy data using strcpy(). (To know the reason, please refer to the link above.) Your corresponding freeEntry() function also wouldn't need to free those members.

Now, for the rest of your program. Your way of finding a terminating newline works, but you can simplify it by using strchr() standard C function. The call in your case would look like:

char *nl;
if ((nl = strchr(first, '\n')) != NULL) {
    *nl = '\0';
}

Finally, when you fix all the above, you will find that you are getting the last record twice in your phone book. In C, feof() doesn't tell you if you are at the end of file now: it tells you that the last attempt to read from the file failed because you were at end of the file.