patterncMinor
Implementing a linked list
Viewed 0 times
listimplementinglinked
Problem
I am studying data structures at the moment, so I want to see if there is anything wrong with my implementation of linked lists in C, after I checked Implementing an ArrayList.
Header
Source
```
#include
#include "LinkedList.h"
#define VALID_LINKEDLIST_CODE (245)
#define _LinkedList_check(l) \
if ((l) == NULL || (l)->_Valid != VALID_LINKEDLIST_CODE) \
abort();
struct _linkedlist {
size_t depth;
int _Valid;
void * value;
struct _linkedlist *next;
struct _linkedlist *previous;
};
struct _linkedlist LinkedList_create(void initial_value)
{ / Allocate Memory /
struct _linkedlist *list = malloc(sizeof(struct _linkedlist));
if(list == NULL)
return NULL;
list->depth = 1;
list->next = NULL;
list->previous = NULL;
list->value = initial_value;
list->_Valid = VALID_LINKEDLIST_CODE;
return list;
}
void LinkedList_set_data(struct _linkedlist * list, void ** d
Header
#ifndef _LINKEDLIST_H
#define _LINKEDLIST_H
#include /* size_t */
#include /* _Bool */
#define _LINKEDLIST_DEFAULT_SIZE (1)
typedef struct _linkedlist _LinkedList;
typedef const unsigned int Index;
#define LinkedList _LinkedList *
LinkedList LinkedList_create();
int LinkedList_add(LinkedList, void *);
int LinkedList_get_val_index(LinkedList, void *);
int LinkedList_get_list_index(LinkedList, const LinkedList *);
int LinkedList_remove(LinkedList, const _Bool, const _Bool);
void LinkedList_set_data(LinkedList, void **, const _Bool, const size_t max);
void LinkedList_clear(LinkedList, const _Bool);
void LinkedList_destroy(LinkedList *, const _Bool);
void *LinkedList_get_value(const LinkedList);
LinkedList LinkedList_get(LinkedList, Index);
LinkedList *LinkedList_get_next(LinkedList);
LinkedList *LinkedList_get_previous(LinkedList);
LinkedList *LinkedList_get_pointer(LinkedList *, Index);
inline size_t LinkedList_get_size_of(const LinkedList);
inline size_t LinkedList_get_size(const LinkedList);
#endif /* _LINKEDLIST_H */Source
```
#include
#include "LinkedList.h"
#define VALID_LINKEDLIST_CODE (245)
#define _LinkedList_check(l) \
if ((l) == NULL || (l)->_Valid != VALID_LINKEDLIST_CODE) \
abort();
struct _linkedlist {
size_t depth;
int _Valid;
void * value;
struct _linkedlist *next;
struct _linkedlist *previous;
};
struct _linkedlist LinkedList_create(void initial_value)
{ / Allocate Memory /
struct _linkedlist *list = malloc(sizeof(struct _linkedlist));
if(list == NULL)
return NULL;
list->depth = 1;
list->next = NULL;
list->previous = NULL;
list->value = initial_value;
list->_Valid = VALID_LINKEDLIST_CODE;
return list;
}
void LinkedList_set_data(struct _linkedlist * list, void ** d
Solution
Your list is a little unusual in that it contains a
validity check field in each list node. I've never see this and think that
the depth in particular is a mistake. Your function must traverse the whole
list adjusting the
Equally when deleting items you must traverse the remaining items. This means
that the list gets slower as it expands. This is not a good design.
I think it would be better to remove
the list head a different structure that holds a pointer to the end of the
list.
Some other observations:
-
Your
-
Avoid using a leading underscore. It's use is reserved. And it adds nothing
to your code.
-
It is often thought best to enclose single-line conditional statements or
loops in braces:
There is no universal agreement on this, but it does help to prevent a
class of errors that can occur if the braces are omitted. Also empty
loops are often thought better if coded thus:
-
In
for NULL pointers. If it doesn't find one it will repeat indefinitiely (on
into undefined memory). I don't know why this function would ever be
necessary.
-
The function is also written with double pointers, which is ugly and
unnecessary.
-
I don't see the point of having both
-
I see no use in
the amount of memory occupied by the non-contiguous (opaque) nodes? Returning the
number of nodes might have more use (
-
list after that node. And it seems to change the depth counter of the head
of the list but not of other nodes.
... I went no further.
depth field and avalidity check field in each list node. I've never see this and think that
the depth in particular is a mistake. Your function must traverse the whole
list adjusting the
depth fields of each item whenever and item is added.Equally when deleting items you must traverse the remaining items. This means
that the list gets slower as it expands. This is not a good design.
I think it would be better to remove
depth from each node and perhaps makethe list head a different structure that holds a pointer to the end of the
list.
Some other observations:
-
Your
linkedlist_check macro should be uppercase and should use a do {...}
while(0) and not end in a semi-colon. -
Avoid using a leading underscore. It's use is reserved. And it adds nothing
to your code.
-
It is often thought best to enclose single-line conditional statements or
loops in braces:
if (length < max || max == 0) {
abort();
}There is no universal agreement on this, but it does help to prevent a
class of errors that can occur if the braces are omitted. Also empty
loops are often thought better if coded thus:
for(length = 0; data[length]; ++length) {
/* nothing */
}-
In
LinkedList_set_data the first for-loop seems to search the data arrayfor NULL pointers. If it doesn't find one it will repeat indefinitiely (on
into undefined memory). I don't know why this function would ever be
necessary.
-
LinkedList_add does not test for failure in calls to LinkedList_create.The function is also written with double pointers, which is ugly and
unnecessary.
-
I don't see the point of having both
LinkedList_get andLinkedList_get_pointer. Also the latter doesn't increment i.-
I see no use in
LinkedList_get_size_of. Why would a caller want to knowthe amount of memory occupied by the non-contiguous (opaque) nodes? Returning the
number of nodes might have more use (
LinkedList_get_size).-
LinkedList_remove is supposed to remove just one node but 'clears' thelist after that node. And it seems to change the depth counter of the head
of the list but not of other nodes.
... I went no further.
Code Snippets
if (length < max || max == 0) {
abort();
}for(length = 0; data[length]; ++length) {
/* nothing */
}Context
StackExchange Code Review Q#64772, answer score: 4
Revisions (0)
No revisions yet.