Linked List in C

Question

I am having some trouble running this linked list implementation (containing words as data). Problem is when I try to print the words (that I inserted) in the Linked List,I get nothing. What am I doing wrong, breaking my head over this? I hope it's not something silly. Anyway here's the code -

typedef struct node
{
    void *data;
    struct node *next;
} NODE;

NODE *new_node(void *data)
{
    NODE *new = malloc(sizeof(NODE));
    if(new)
    {
        new->data = data;
        new->next = NULL;
        return new;
    }
    else
    {
        return NULL;
    }
}

void print_list(NODE *head, void (print_fn) (void*))
{
    if(head && head->next)
    {
        while(head->next)
        {
            if(print_fn)
                print_fn(head->data);
            else
                printf("Word: %s\n", (char *)head->data);
            head = head->next;
        }
    }
    return;
}

void append(NODE **head, NODE *node)                                                                                  
{
    NODE *tmp = *head;
    if(tmp && node)
    {
        while(tmp->next)
            tmp = tmp->next; 
        (*head)->next = node; /*add as last node*/
    }
    return;
}


NODE *create_list()
{
    FILE *dict_file = fopen("trial.txt", "r");

    if(dict_file)
    {
        NODE *head = new_node(NULL);
        if(!head) return NULL;

        char word[20];
        int first  = TRUE;
        memset(word, '\0', 20);

        while(fgets(word, sizeof(word), dict_file) != NULL )
        {
            if(first)
            {
                head->data = (void*)word;
                first = FALSE;
            }
            else
            {
                append(&head, new_node((void*)word));
            }
        }
        fclose(dict_file);
        return head;
    }
    else
    {
        printf("ERROR: File not found");
        return NULL;
    }
}

int main(int argc, char *argv[])
{
    NODE *head = create_list();

    if(!head)
    {
        printf("ERROR: Either malloc() failed or data not found\n");
        return FALSE;
    }
    else
    {
        print_list(head, NULL);
        return TRUE;
    }
}

Answer 1

This has become quite a long answer. Don't take this personally, but you have made quite a few rookie mistakes. I have met with many people at university whom I helped to learn the C language and programming in general, so I've got used to notice these kinds of things.

The important issues I could find

• You assign a pointer to a stack variable to words
  This is completely bad, because that value will be overwritten once the execution gets out of the function that creates it. Solution: copy the content of that variable into a heap variable.

• Your append function is faulty
  It adds the appended element to the second place instead of the last place. Note that you don't need the return at the end either. There is also no point in requiring a double-pointer as input to the append method. Also, after assigning head to tmp, it is futile to check tmp against NULL as well, since it won't be NULL if head isn't NULL. Also, I recommend checking the new node against NULL as well. If it is NULL, this saves you from iterating over the entire collection.

• The create_list function is sub-optimal
  Fist, the distinction between the first and the other cases are futile. Introducing another pointer (called current in my code) will eliminate the need to check whether it is the first or not. Next, you always call the append function on the head, thus you always need to iterate over the entire collection. This can be also optimized by introducing the current variable. (Which, at start, should be assigned the value of head.)

• The print_list function is errornous
  It doesn't print anything if there is only one node. It also redundantly checks the pointers for null. (The beginning of the loop checks it, too.) The return statement at the end of this void function is also unnecessary.

• You should free up memory when you don't use it
  @Baltasarq wrote a nice clear function in his answer, you should use it. :)

Not serious errors, but you should be aware of them

• You should not use void* instead of char* If you know that the data member of the NODE structure is going to store characters, why do you use void*? It is bad practice! (Unless you have a good reason, of course.)

• Using the new word as a variable name makes your code not compliant with C++. Thus, I recommend against it.

• Adopt a better coding style, please - it will make your code so much easier to read

• Inconsistency: if in print_list you don't allocate a new variable to go through the collection (like you do with the tmp variable in append) then it is misguiding to name the parameter as head. (I renamed it to node in my code.)

Here is the fixed code

(Note that there may be small syntax errors because I typed the code into the browser without actually testing it.)

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

typedef struct node
{
    void *data;
    struct node *next;
} NODE;

NODE *new_node(void *data)
{
    NODE *newNode = (NODE*)malloc(sizeof(NODE));
    if (newNode)
    {
        newNode->data = data;
        newNode->next = NULL;
        return newNode;
    }
    return NULL;
}

void append(NODE *head, NODE *node)
{
    if (head && node)
    {
        NODE *tmp = head;
        while (tmp->next)
            tmp = tmp->next; 
        tmp->next = node; /* add as last node */
    }
}

void print_list(NODE *node, void (print_fn) (void*))
{
    while (node)
    {
        if (print_fn)
            print_fn(node->data);
        else
            printf("Word: %s\n", (char *)node->data);

        node = node->next;
    }
}

NODE *create_list()
{
    FILE *dict_file = fopen("trial.txt", "r");

    if (dict_file)
    {
        NODE *head = NULL;
        NODE *current = head;

        char word[20];
        memset(word, '\0', 20);

        while (fgets(word, sizeof(word), dict_file))
        {
            // Creating a variable on the heap
            char *data = calloc(sizeof(word) + 1, sizeof(char));
            // Copying the contents of words to it
            strcpy(data, word);

            append(current, new_node((void*)data));
            if (current->next)
                current = current->next
        }
        fclose(dict_file);
        return head;
    }
    else
    {
        printf("ERROR: File not found");
    }
    return NULL;
}

int main(int argc, char *argv[])
{
    NODE *head = create_list();

    if (!head)
    {
        printf("ERROR: Either malloc() failed or data not found\n");
    }
    else
    {
        print_list(head, NULL);
    }
    return 0;
}

Answer 2

Be careful, since malloc() and derivatives can answer with a NULL pointer when there is not enough memory. In fact, take into account that you'll also need a clear() method that free's all the data, as well as the nodes themselves.

void clear(NODE *node)
{
    NODE * temp = NULL;

    while( node != NULL ) {
       temp = node->next;
       free( node->data );
       free( node );

       node = temp;
    }
}

Your main() function should return an exit code to the operating system of EXIT_SUCCESS or EXIT_FAILURE, instead of TRUE or FALSE.

int main(int argc, char *argv[])
{
    NODE *head = create_list();
    int toret = EXIT_SUCCESS;

    if(!head)
    {
        printf("ERROR: Either malloc() failed or data not found\n");
        toret = EXIT_FAILURE;
        clear( head );
    }
    else
    {
        print_list(head, NULL);
        clear( head );
    }

    return toret;
}