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

Singly-linked implementation in C

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

Problem

I'm a C++ programmer and thought I'd revisit the roots, so I scribbled something really quickly. I would appreciate input on how my C code is looking!

slist.h

#ifndef SLIST_H
#define SLIST_H

#include 
#include 

struct slist_s 
{
    int key;
    struct slist_s* next;
};

typedef struct slist_s slist_t;

slist_t* slist_push_front(slist_t*, int);
slist_t* slist_push_back(slist_t*, int);

slist_t* slist_pop_front(slist_t*);
slist_t* slist_pop_back(slist_t*);

_Bool    slist_empty(const slist_t* const);
slist_t* slist_clear(slist_t*);

size_t   slist_size(const slist_t*);

void     slist_print(const slist_t*, const char*);

slist_t* slist_for_each(slist_t* const, void(*)(int*));

#endif


slist.c

```
#include
#include
#include
#include

#include "slist.h"

slist_t slist_push_front(slist_t root, int val)
{
slist_t* node = malloc(sizeof(slist_t));
if (!node) {
return NULL;
}
node->key = val;
node->next = !root ? NULL : root;

root = node;
return root;
}

slist_t slist_push_back(slist_t root, int val)
{
slist_t* node = malloc(sizeof(slist_t));
if (!node) {
return NULL;
}
node->key = val;

if (!root)
{ / list is empty /
node->next = NULL;
root = node;
}
else
{ / need to walk to end of list /
slist_t* temp = root;
while (temp->next) {
temp = temp->next;
}
node->next = NULL;
temp->next = node;
}
return root;
}

slist_t slist_pop_front(slist_t root)
{
if (!root) {
return NULL;
}
if (!root->next)
{
free(root);
return NULL;
}
slist_t* todel = root;
root = root->next;
free(todel);
return root;
}

slist_t slist_pop_back(slist_t root)
{
if (!root) {
return NULL;
}
if (!root->next)
{
free(root);
return NULL;
}
slist_t* todel = root;
while (todel->next->next) {
todel = todel-

Solution

-
Returning root

Returning the root suggests the use case

root = slist_push_front(root, value);


However, if node allocation fails, the return value is NULL, and the whole list is lost. I recommend to change the signature to

int slist_push_front(slist_t ** rootptr, int value);


to be used along the lines of

if (slist_push_front(&root, value) == FAIL) {
        ....
    }


-
In slist_push_front, it doesn't matter whether the root is NULL or not. node->next = root will work just fine in both cases.

-
In slist_push_back the next pointer must become null in any case, so it seems logical to assign it once when the node is created.

I also recommend to refactor the while (temp->next) loop into a slist_get_tail function. Then slist_push_back is streamlined into

int slist_push_back(slist_t ** root, int val)
{
    slist_t * node = slist_create_node(val);
    if (node == NULL) {
        return FAILURE;
    }
    slist_t * tail = slist_get_tail(*root);
    if (tail) {
        tail->next = node;
    } else {
        *root = node;
    }

Code Snippets

root = slist_push_front(root, value);
int slist_push_front(slist_t ** rootptr, int value);
if (slist_push_front(&root, value) == FAIL) {
        ....
    }
int slist_push_back(slist_t ** root, int val)
{
    slist_t * node = slist_create_node(val);
    if (node == NULL) {
        return FAILURE;
    }
    slist_t * tail = slist_get_tail(*root);
    if (tail) {
        tail->next = node;
    } else {
        *root = node;
    }

Context

StackExchange Code Review Q#117609, answer score: 2

Revisions (0)

No revisions yet.