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

Singly Linked List

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

Problem

I am a self taught programmer and, while I understand data structures at a theoretical level (as well as through optimizing my own code), I have never actually taken the time to write my own implementation of all the major ones.

So, here is my linked list. I would appreciate any feedback you guys can offer to me, especially when it comes to cleaner/faster/more idiomatic way of implementing some of these basic algorithms (for example, insert). I didn't look at any reference material because I wanted to see how close I would get without help. As such, I'm sure there are probably simpler ways of doing some things that I overlooked.

Also note that I have not optimized any of this; this is simply a first pass, took about an hour. I will be benchmarking this (and the others) at a later time, but like I said, space and time improvements are welcome.

linked_list.h

#ifndef LINKED_LIST_H
#define LINKED_LIST_H

#include 

typedef struct _node {
    int value;
    struct _node* next;
} node;

typedef struct _llist {
    node* head;
    node* tail;
} llist;

void init_node( node** new_node, int value );
void init_llist( llist** list );
size_t len( llist* list );
void push_back( llist* list, int value );
void push_front( llist* list, int value );
void insert( llist* list, int index, int value );
node* find( llist* list, int value );
void free_llist( llist* list );
void print_llist( llist* list );

#endif


linked_list.c

```
#include "linked_list.h"
#include
#include

void init_node( node** new_node, int value ) {
new_node = (node)malloc( sizeof(node) );
(*new_node)->value = value;
(*new_node)->next = NULL;
}

void init_llist( llist** list ) {
list = (llist)malloc( sizeof(llist) );
(*list)->head = NULL;
(*list)->tail = NULL;
}

void push_back( llist* list, int value ) {
node* new_node;
init_node( &new_node, value );
if( !list->head ) {
/ empty list /
list->head = new_node;
list->tail = list->head

Solution

Your question is tagged c, and your file ends in .c. So here's some thoughts from that angle:

-
Don't bother casting void to another pointer type. This is a C++ thing. In C, void can be implicitly cast to any pointer type.

-
There is no such thing as a namespace, so don't use collision-prone names like node or len(). Add some kind of prefix.

-
malloc can fail. You will want to bubble up that error status. All your functions that end up calling malloc further down the stack will need a means to return error status as well. Typically the way to do this is return NULL (if your function returns a pointer type) or make the function return bool (` in C99) or int and have some convention where a certain value means failure. (In POSIX it's typically int returning 0 on success. Windows will typically have a boolean where nonzero means success. Returning error codes is also popular.)

Some other thoughts:

-
Consider an alternate allocation scheme for
struct llist. Personally I would prefer the structure itself (not the nodes of course) to be caller-allocated. Is it worth doing a malloc for something only the size of two pointers in llist_init? I would say no. Further, consider the memory layout if you want to include a list inside a structure: why bother having a pointer to it when you can just have the struct llist` be a member of the larger structure?

-
The use of whitespace seems inconsistent. At times you put an extra newline between declarations and statements, and at times you don't. It's of course subjective which one of these you choose (I would rather more space personally), but it's important to be consistent.

Context

StackExchange Code Review Q#5546, answer score: 13

Revisions (0)

No revisions yet.