C: reading text into linked list

Question

Im trying to read into a linked list from a textfile. The text file has the title of the book, author and year separated by ":". each book is on a separate line. the textfile entries look like this:

Absalom, Absalom!:William Faulkner:1936
After Many a Summer Dies the Swan:Aldous Huxley:1939    
Ah, Wilderness!:Eugene O'Neill:1933

i'm rewriting it from scratch. comments would be appreciated.

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

struct BookNode
{
    char linebuffer[128];
    char delim[]=":";
    char * Title[50];
    char * Author[50];
    char * Year[5];
    struct BookNode *next;
//    char *token = NULL;
};

int main(void)
{
    static const char booklist[]= "booklist.txt";
FILE *fr=fopen("booklist.txt", "r");
if ( fr != NULL)

{
char Title[50];
char Author[50];
char Year[5]
struct BookNode Booknode;
while (fgets(linebuffer,128, fr) != NULL &&
    sscanf(line, "%49s %49s %4s", 
        &BookNode.Title, BookNode.Author, BookNode.Year)==3)
    {
         printf("%50s %50s %5s", 
                BookNode.Title, BookNode.Author, BookNode.Year);
    }
}

Answer 1

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

struct BookNode {
    char * Title;
    char * Author;
    char * Year;
    struct BookNode * next;
} * head;


void addEntry(char * T, char * A, char * Y);
void display();
int numEntries();
//void writeBookData(struct BookNode * selection);
void free_book(struct BookNode *bnp){
    if(bnp == NULL) return;
    free(bnp->Title);
    free(bnp->Author);
    free(bnp->Year);
    free_book(bnp->next);
    free(bnp);
}

int main() {
    FILE * fpointer;
    fpointer=fopen("booklist.txt","r");
    if(fpointer == NULL){
        printf("Booklist could not be opened.\n");
        exit(EXIT_FAILURE);
    }

    char Title[50+1];
    char Author[50+1];
    char Year[4+1];

    head = NULL;
    while (EOF!=fscanf(fpointer, "%50[^:]%*c%50[^:]%*c%4[^\n]%*c", Title, Author, Year)){
        //note:The input number of characters is limited (Eg50), it (because minutes in excess of the limit  is used in the following items) there must be large enough.

        addEntry(Title, Author, Year);
    }
    fclose(fpointer);

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

    display();

    free_book(head);
    return 0;
}

void addEntry(char * T, char * A, char * Y){
    struct BookNode * tempNode, * iterator;
    tempNode = (struct BookNode *)malloc(sizeof(struct BookNode));
    tempNode->Title = (char *)malloc(strlen(T)+1);
    strcpy(tempNode->Title, T);

    tempNode->Author = (char *)malloc(strlen(A)+1);
    strcpy(tempNode->Author, A);

    tempNode->Year = (char *)malloc(strlen(Y)+1);
    strcpy(tempNode->Year, Y);

    tempNode->next = NULL;

    iterator = head;

    if (head == NULL){
        head = tempNode;
    } else {
        while(iterator->next != NULL){
            iterator = iterator->next;
        }
        iterator->next = tempNode;
    }
}

int numEntries(){
    if(head == NULL)
        return 0;
    else{
        int count;
        struct BookNode *iterator;
        for(count=0, iterator=head; iterator!=NULL; iterator = iterator->next, ++count)
            ;
        return count;
    }
}

void display(){
    if(head == NULL)
        return ;
    else{
        struct BookNode *iterator;
        for(iterator=head; iterator!=NULL; iterator = iterator->next)
            fprintf(stdout, "%s:%s:%s\n", iterator->Title, iterator->Author, iterator->Year);
    }
}

Answer 2

example of strtok

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

int main(){
    char line[] = "Absalom, Absalom!:William Faulkner:1936\n";
    char *p;
    char * Title;
    char * Author;
    char * Year;

    p = strtok(line, ":");
    Title = strdup(p);
    Author = strdup(strtok(NULL, ":"));
    Year = strdup(strtok(NULL, ": \n"));
    printf("\"%s\",\"%s\",\"%s\"\n", Title, Author, Year);
    free(Title);
    free(Author);
    free(Year);
}
//result:"Absalom, Absalom!","William Faulkner","1936"

Answer 3

There are multiple problems in your code right now.

The first one (I kid you not) is with code formatting and indentation. Your pasted sample didn't have a regular format or indentation to speak of. It's more difficult to follow code flow even in short samples such as this. Always indent your code, and pick a coding style (there are several) and stick to it.

Regarding code flow, the first problem is in error checking. Namely, you check for fopen return status, but do not take sufficient action should opening a file fail.

The second problem is a conceptual one. You don't seem to realise that an array of N characters can only hold a string with a lenght of N-1. Therefore, char[4] is hardly ever a suitable format for storing years as strings.

Now that those issues have been dealed with, here are the actual flaws that would prevent your code from working in any case:

1) The fgets function will read up until it either fills your buffer or reaches an end-of-line or an end-of-file character. Yet you still call fgets thrice to try and read a single-line entry in your file. It's unlikely what you want to do. You have to re-think the contents of your loop.

2) Your "main" loop condition is likely to be flawed. This is a very common misunderstanding of the use of feof & co. Assuming your data file contains a newline at the end (and it would only be regular for it to do so), your loop will execute one time too many.

It's better to structure your line reading loops like this:

while (fgets(buffer, BUF_SIZE, stdin)) { /* parse buffer */ }

3) You have elementary problems with memory management in your code: namely, the function addEntry fails to allocate memory to store your records. Instead, all entries in your linked list will end up pointing to the same shared buffer you allocate in your main function.

There are several ways to fix this. One is to use several calls to malloc for each member of your BookNode structs (title, author, and year). Another, perhaps preferable method is to use variable-size structs, like this:

struct BookNode {
    char *title;
    char *author;
    char *year;
    struct BookNode *next;
    char buffer[]; // this shorthand requires C99
};

For each struct BookNode you allocate enough storage after them, so that you can copy there the contents of your shared buffer. title, author, and year then point to this appended storage. This way you won't end up overwriting the contents of other BookNodes in the next iteration of your loop. And you only need one free to free an entire node.

I probably didn't list all the problems in your code here. Perhaps instead of another rewrite, you should first try to tackle a smaller subproblem such as reading a single entry from stdin and building up from there?

Answer 4

addEntry should allocate memory for title, author and year.
Also, doing fgets three times would read 3 lines. You need one fgets per loop, and split the result to different parts (e.g. using strtok_r).

What you do is save a pointer to a static buffer. When reading the next line, this buffer is overwritten with new data.

Note that if you allocated data, you must eventually free it. The entry's destructor will need to free.