patterncMinor
Singly-linked implementation in C
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
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-
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*));
#endifslist.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
Returning the root suggests the use case
However, if node allocation fails, the return value is NULL, and the whole list is lost. I recommend to change the signature to
to be used along the lines of
-
In
-
In
I also recommend to refactor the
Returning
rootReturning 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 intoint 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.