Linked list in C [closed]

One key problem is:

 if(mylist->head  = NULL){

First, you've not checked whether mylist itself is NULL, and on the first call to slist_insert(), it is NULL. That alone leads to trouble.

Second, you've got an assignment in there instead of a comparison; you need a comparison.

When it comes down to it, you need to handle the empty list case and not mylist->head == NULL. The code inside the if (mylist->head = NULL) is extremely problematic. You allocate two node values, then you overwrite the pointers with newnode, leaking memory. The code does not set mylist->len either. You have a similar memory leak in the else clause.

if (mylist == NULL)
{
    mylist = malloc(sizeof(*mylist));  // or sizeof(struct slist)
    mylist->head = newnode;
    mylist->tail = newnode;
    mylist->len  = 1;
}
else
{
    mylist->tail->next = newnode;
    mylist->tail = newnode;
    mylist->len++;
}

You have a number of places where you leave spaces around the -> operator; don't! It bind very tightly and should never have spaces around it. Ditto for the . operator.

Do not write void main(). It isn't valid in standard C; it isn't valid in Microsoft C. Further, since Microsoft C is based on C89, you must also include a return 0; at the end of main(). In C99 and C11, that is optional (though that is IMNSHO a bug in the standard) and you are best off always using return 0; or equivalent at the end of main().

In the main() function, you don't need the allocation.

struct snode *temp = mylist->head;
while (temp != NULL)
{
    printf("%d\n", temp->val);
    temp = temp->next;
}

You should not have an empty declaration after the main() program — the semicolon after the close brace is technically an empty declaration. Omit the semicolon. Ditto after slist_insert().

You don't need the empty statement (semicolon) after the while loop in main(), either. Nor do you need the empty statement after the else block in slist_insert(). These don't do much harm, but they don't do any good either.

At some point, you need to add the code to release the list.

Your code is not very general. You are tied to a single list. You should be able to create an empty list, and then pass that list to slist_insert() so you could have multiple lists concurrently. Global variables (like mylist) are bad when they can be avoided — and this one could be avoided. However, this is a secondary issue; you have a lot of primary issues to resolve first.

An alternative design would use struct slist mylist; as the global variable, avoiding the need to dynamically allocate the list. However, that doesn't work well for the generalized version, so I don't recommend implementing it (except as an exercise).

Personally, I'd create types for the structures:

typedef struct snode snode;
typedef struct slist slist;

and I'd use those in the code. I haven't implemented that.

Your code does not check that malloc() returns a non-null pointer; neither does mine, but that is lazy. However, you also don't have an easy way to indicate the problem with the current interface — other then a unilateral exit from the program. I've used assert() statements to deal with this. I've also tagged the lines 'reprehensible' because using assert() for checking memory allocation is a very bad idea — only marginally less bad than not checking at all.

Here is working code incorporating most of the points raised above (and probably a few other minor details). I left the debugging code I added while sorting out the problems; you can see what I used and work out why. Every print except the one in the while loop in main() is debugging code.

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

struct snode
{
    struct snode *next;
    int val;
};

struct slist
{
    struct snode *head;
    struct snode *tail;
    int len;
};

struct slist *mylist;

extern void slist_insert(int v);
extern void slist_destroy(void);

void slist_insert(int v)
{
    struct snode *newnode = (struct snode*)malloc(sizeof(struct snode));
    assert(newnode != NULL);  // Reprehensible
    newnode->val = v;
    newnode->next = NULL;

    if (mylist == NULL)
    {
        mylist = malloc(sizeof(*mylist));
        assert(mylist != NULL); // Reprehensible
        mylist->head = newnode;
        mylist->tail = newnode;
        mylist->len  = 1;
    }
    else
    {
        assert(mylist->head != 0);
        assert(mylist->tail != 0);
        assert(mylist->tail->next == 0);
        mylist->tail->next = newnode;
        mylist->len++;
        mylist->tail = newnode;
    }
}

void slist_destroy(void)
{
    struct snode *curr = mylist->head;
    while (curr != NULL)
    {
        printf("Free: %d\n", curr->val);
        struct snode *next = curr->next;
        free(curr);
        curr = next;
    }
    printf("Free: mylist\n");
    free(mylist);
    mylist = 0;
}

int main(void)
{
    printf("Before 1\n");
    assert(mylist == 0);
    slist_insert(1);
    assert(mylist != 0);
    printf("Before 2\n");
    slist_insert(2);
    printf("Before 3\n");
    slist_insert(3);
    printf("Before 4\n");
    slist_insert(4);
    printf("Before 5\n");
    slist_insert(5);

    printf("Before loop\n");

    struct snode *temp = mylist->head;
    while (temp != NULL)
    {
        printf("%d\n", temp->val);
        temp = temp->next;
    }

    slist_destroy();

    return 0;
}