patterncModerate
Singly Linked List
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,
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
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
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 );
#endiflinked_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
-
Don't bother casting
-
There is no such thing as a namespace, so don't use collision-prone names like
-
-
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.
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.