0

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);
    }
}
4
  • Would you be so kind to give a better explaination on text file format? Commented Jun 3, 2012 at 12:19
  • Mind pasting the addEntry function? I see the prototype, but not the function Commented Jun 3, 2012 at 12:19
  • I've added extra info on the text file. thats all i have so far. its an amalgamation of all the stuff that ive been reading so far. i am not confident in this at all:\ Commented Jun 3, 2012 at 12:27
  • 1
    1) your use of feof() is wrong. 2) the addEntry() function assigns the address of automatic ("stack") variables (from the caller)to the returned struct. You should probably strdup() them. These pointers will point into the same {Title,Author,Year} buffers every time; so they will be overwritten on every iteration of the main() loop. 3) fgets reads an entire line , including '\n' on every call. Commented Jun 3, 2012 at 13:29

4 Answers 4

5

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?

Sign up to request clarification or add additional context in comments.

Comments

0

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.

3 Comments

im sorry but i dont quite follow.
You want a linked list, so you allocate memory for each item. So far so good. The items contain pointers, but what do these pointers point to? You want each item to have different pointers, pointing to different data. So you must allocate memory for it.
i've made changes to the code to allocate space. i do not know if i did it the right way though
0

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"

Comments

0
#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);
    }
}

1 Comment

thanks BLUEPIXY! that should get me going. just need to figure out how to remove ":" and then make changes to the Linked list

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.