File data to a linked list in C

Question

I am really close here.

This reads the file in and using gdb I can see the words from my text file going to the linked list.

However when I print my linked list (bottom part of code) the whole linked list seems to just contain the last word from the file duplicated for however many entries are in the file.

bool load(const char* dictionary) {
    // open dictionary
    FILE* file = fopen(dictionary, "r");
    if (file == NULL)
        return false;

    char buf[LENGTH];

    //read the contents of the file and write to linked list
    while (!feof(file)) {
        fscanf(file,"%s", buf);

        // try to instantiate node
        node* newptr = malloc(sizeof(node));
        if (newptr == NULL) {
            return false;
        }

        // initialize node
        newptr->next = NULL;

        // add new word to linked list
        newptr->dictword = buf;

        //move node to start of linked list
        newptr->next = first;
        first = newptr;
    }

    fclose(file); 

    // traverse and print list.
    node* ptr = first;
    while (ptr != NULL) {
        printf("%s\n", ptr->dictword);
        ptr = ptr->next;
    }

    return true;
}

You are using `feof` incorrectly: http://stackoverflow.com/questions/5431941/while-feof-file-is-always-wrong/5432517#5432517 — William Pursell, Feb 27 '13 at 23:16
bool is a terrible return type for this function. Why don't you just return a pointer to the start of the list (or NULL) ? — wildplasser, Feb 27 '13 at 23:18
fscanf(file,"%s",buf), might easily lead to a bufferoverflow — hetepeperfan, Feb 27 '13 at 23:20
Why might this lead to a buffer overflow? I think it has when my dictionary gets bigger. — Dan Heaford, Mar 02 '13 at 22:16

score 1 · Answer 1 · answered Feb 27 '13 at 23:16

You only have one char buf[] into which you read every word. You save a pointer to that buffer in every linked list element, but you immediately read over the data. You need to copy it when you put it into the allocated node. The easiest way is to use newptr->dictword = strdup(buf) which does the alloc and copy in one step.

Tuxdude · Answer 2 · 2013-02-27T23:24:53.863

Assuming dictword is of size LENGTH just as your buf, i.e:

struct node {
...
...
    char dictword[LENGTH];
...
...
};

you need to make the following changes in your code:

strcpy(newptr->dictword, buf);

The problem was that you were just setting the dictword to point to the same memory as buf in all the nodes of the linked list.

A better approach would be to dynamically allocate the memory for the string:

struct node {
...
...
    char* dictword;
...
...
};

newptr->dictword = (char*)malloc(strlen(buf) + 1);
strcpy(newptr->dictword, buf);

score 0 · Answer 3 · answered Feb 27 '13 at 23:19

0

I recommend strdup solution as BenJackson's. But it does not exist in the standard C library.

Here is my workaround for it:

char *strdup(const char *data)
{
   char *retval = malloc(strlen(data) + 1);
   strcpy(retval, data);
   return retval;
}

answered Feb 27 '13 at 23:19

Aniket Inge

24,753
5
47
78

Dmitry · Answer 4 · 2013-02-27T23:41:00.407

0

using feof indicates you are 1. not using unix environment and having to accomodate to mingw's inability to eof properly in stdin. 2. overthinking and should actually be checking for eof.

I usually do this:

void* collection = ...;
FILE* filebehere = ...;
char buffer[1337];
while (fgets(buffer, 1337, filebehere)) {
    if (strlen(buffer) != 0) {
        buffer[strlen(buffer) - 1] = 0;
        collection_insert(collection, buffer);
    }
}

edited Feb 27 '13 at 23:41

answered Feb 27 '13 at 23:25

Dmitry

4,752
4
35
48

`buffer[strlen(buffer)] = 0;` is a noop. ("find the place where the first '\0' is, and put a '\0' there" ) – wildplasser Feb 27 '13 at 23:28
youre right thanks for the correction, fixing. It should be strlen(buffer) - 1 to remove the '\n' from the string. Once the strings are loaded, nothing is worse than having to either strip '\n' one by one, or having to compare with explicit '\n's – Dmitry Feb 27 '13 at 23:32
That is even worse. Strlen() could return zero. – wildplasser Feb 27 '13 at 23:34
There could be a NUL as the first character on a line, even before the '\n'. The file does not *have* to be "clean" text ascii. – wildplasser Feb 27 '13 at 23:39
you're right, i made an if statement to ensure it skips nul lines, this is likely to help avoid breaking on the last line of the file. – Dmitry Feb 27 '13 at 23:41
Now you are calling strlen() two times. Better use a variable. Also: the line does not *have* to be '\n' terminated. There could be a line without a CR at the end of file. BTW: feof() is not a matter of mingw's *disability*, it just the way it is standardised. calling feof() without a previous failing read is just plain wrong. – wildplasser Feb 27 '13 at 23:43
there is never a cr at the end of the file, theres an EOF. and putting that in a variable would be optimal, but I see no reason to add additional complexity for the sake of demonstration, the question is of functionality at this point, not efficiency. – Dmitry Feb 27 '13 at 23:45

score 0 · Answer 5 · answered Feb 27 '13 at 23:28

There is a problem with the line:

// add new word to linked list
newptr->dictword = buf;

In this way you are allways passing the reference to the beginning of the buf to all your elements, so as you update the "buf" all elements are updated. And that's why you have the referred behaviour.

Try replace it by:

newptr->dictword = (char*) malloc((strlen(buf)+1)*sizeof(char));
strcpy(newptr->dictword, buf);

The "strlen(buf)+1" is the length of the current string stored in buf plus the space for the character "\0", which means end of string.

PS. assuming you have declared the elements "dictword" with char*.

File data to a linked list in C

5 Answers5