HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Linked List simple implementation

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
listimplementationlinkedsimple

Problem

I made a simple linked list in C with the following functionalities:
Create(creates the list); Find(searches for an element in the list); Insert(inserts a value at the beginning of the list); Destroy(destroys the entire list). It only uses integers (for simplicity), and I want to know for possible errors and how to improve it in general.

The node structure:

typedef struct sllist
{
    int val;
    struct sllist* next;
}
sllnode;


The functions:

#include 
#include 
#include "node.c"

/*
 *Creates the linked list with a single value
*/
sllnode *create(int value)
{
    sllnode *node;
    node = (sllnode *) malloc(sizeof(sllnode));

    if (node == NULL)
    {
        puts("Memory error");
        return NULL;
    }

    node->val = value;
    node->next = NULL;

    return node;
}

/*
 *Searches for an element in the list.
 *Returns 1 if its found or 0 if not
*/
int find(sllnode *head, int value)
{
    sllnode *trav = head;

    do
    {
        if (trav->val == value)
        {
            return 1;
        }
        else
        {
            trav = trav->next;
        }
    } while (trav != NULL);

    return 0;
}

/*
 *Inserts a value at the beginning of the list
*/
sllnode *insert(sllnode *head, int value)
{
    sllnode *new_node = (sllnode *) malloc(sizeof(sllnode));

    if (new_node == NULL)
    {
        puts("Memory error");
        return NULL;
    }

    new_node->val = value;
    new_node->next = head;
    head = new_node;

    return head;
}

/*
 *Destroys (i.e frees) the whole list
*/
void *destroy(sllnode *head)
{
    sllnode *temp = head;

    while (temp != NULL)
    {
        free(temp);
        temp = temp->next;
    }
    free(head);
}

int main(void)
{
    return 0;
}

Solution

Including a C file

Convention is that all shared stuff is declared in .h files.

Minor 1

In destroy(), you have the return type void*. Remove the pointer star and it's fine.

Minor 2

sllnode *create(int value)
{
    sllnode *node;
    node = (sllnode *) malloc(sizeof(sllnode));

    if (node == NULL)
    {
        puts("Memory error");
        return NULL;
    }
    ...


It's a bad idea to abuse standard output from within algorithms/data structures. After all, everything that may go wrong is that there is no memory enough for the new node; returning NULL may well indicate that situation.

Minor 3

node = (sllnode *) malloc(sizeof(sllnode));


More idiomatic C is

node = malloc(sizeof *node);


Minor 4

You "append" the new nodes in reversed direction. Basically this is a (linked) stack and not a list.

Minor 5

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


Don't do this. Instead have something like

while (temp) 
{
    next_node = temp->next;
    free(temp);
    temp = next_node;
}


Summa summarum

For a starter, I had this in mind:

#include 
#include 

typedef struct linked_list_node {
    int value;
    struct linked_list_node* next;
} linked_list_node;

typedef struct {
    linked_list_node* head;
    linked_list_node* tail;
} linked_list;

void linked_list_init(linked_list* list)
{
    if (!list)
    {
        return;
    }

    list->head = NULL;
    list->tail = NULL;
}

void linked_list_free(linked_list* list)
{
    linked_list_node* current_node;
    linked_list_node* next_node;

    if (!list)
    {
        return;
    }

    current_node = list->head;

    while (current_node)
    {
        next_node = current_node->next;
        free(current_node);
        current_node = next_node;
    }
}

int linked_list_append(linked_list* list, int value)
{
    linked_list_node* new_node;

    if (!list)
    {
        return 1;
    }

    new_node = malloc(sizeof *new_node);

    if (!new_node)
    {
        return 1;
    }

    new_node->value = value;

    if (list->head)
    {
        list->tail->next = new_node;
    }
    else
    {
        list->head = new_node;
    }

    list->tail = new_node;
    return 0;
}

int linked_list_index_of(linked_list* list, int value)
{
    int index;
    linked_list_node* node;

    if (!list)
    {
        return -2;
    }

    for (node = list->head, index = 0; node; node = node->next, index++)
    {
        if (node->value == value)
        {
            return index;
        }
    }

    return -1;
}

int main() {
    int i;

    linked_list my_list;
    linked_list_init(&my_list);

    for (i = 0; i < 10; ++i)
    {
        linked_list_append(&my_list, i);
    }

    printf("%d %d\n",
           linked_list_index_of(&my_list, 4),
           linked_list_index_of(&my_list, 100));

    linked_list_free(&my_list);
    return 0;
}


Hope that helps.

Code Snippets

sllnode *create(int value)
{
    sllnode *node;
    node = (sllnode *) malloc(sizeof(sllnode));

    if (node == NULL)
    {
        puts("Memory error");
        return NULL;
    }
    ...
node = (sllnode *) malloc(sizeof(sllnode));
node = malloc(sizeof *node);
while (temp != NULL)
{
    free(temp);
    temp = temp->next;
}
while (temp) 
{
    next_node = temp->next;
    free(temp);
    temp = next_node;
}

Context

StackExchange Code Review Q#142932, answer score: 2

Revisions (0)

No revisions yet.