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

C linked list implementation

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

Problem

For the following C linked list implementation, please give me some suggestions for improved efficiency, style and design. This implementation is not 100% complete, please give some suggestions for additional features. Notice the data type for the list is void*, I want it to be as generic as possible.

accList.h:

#ifndef ACCLIST_H
#define ACCLIST_H

#include 
#include 

struct accListNode                 //the nodes of a linked-list for any data type
{
  void *data;                     //generic pointer to any data type
  struct accListNode *next;       //the next node in the list
};

struct accList                    //a linked-list consisting of accListNodes
{
  struct accListNode *head;
  struct accListNode *tail;
  int size;
};

void accList_allocate(struct accList *theList);           //allocate the accList and set to NULL
void appendToEnd(void *data, struct accList *theList);    //append data to the end of the accList
void removeData(void *data, struct accList *theList);         //removes data from accList

#endif


accList.c:

```
#include "accList.h"
#include "cmpsc311.h"

void accList_allocate(struct accList *theList) //allocate and initialize to NULL values
{
theList = Malloc(sizeof(struct accList));
theList->head = NULL;
theList->tail = NULL;
theList->size = 0;
}

void appendToEnd(void data, struct accList theList)
{
struct accListNode *newNode = Malloc(sizeof(struct accListNode));
newNode->data = data;
newNode->next = NULL;
if(theList->head == NULL) //the list is empty
{
theList->head = theList->tail = newNode;
}
else //the list is not empty
{
theList->tail->next = newNode;
theList->tail = newNode;
}
}

void removeData(void data, struct accList theList)
{
if(theList->head == NULL) //the list is empty
return;
else if(theList->head == theList->tail) //there is one element in the list
{
free(theList->head);
t

Solution

Several problems I see:

accList_allocate

accList_allocate doesn't do what you probably think. It creates a new accList, yes, but this new one will not be visible to the caller (and thus, will be lost as memory leakage). If you want to initialize the pointer passed to it, use a pointer-to-pointer:

void accList_allocate(struct accList **outList)    //allocate and initialize to NULL values
{
   struct accList* theList = Malloc(sizeof(struct accList));
   theList->head = NULL;
   theList->tail = NULL;
   theList->size = 0;
   *outList = theList;
}


Also, you should really check the return value of Malloc. Especially on embedded systems, there's a real possibility of running out of memory. Let the caller handle the error, but tell him that something's wrong.

appendToEnd

Same as with accList_allocate, check Malloc's return value and return some kind of error code if it's NULL. What you're writing there is library code that should always handle errors gracefully and let the caller decide whether to crash and burn or recover and try again.

removeData

Please refer to my answer in your other thread.

Code Snippets

void accList_allocate(struct accList **outList)    //allocate and initialize to NULL values
{
   struct accList* theList = Malloc(sizeof(struct accList));
   theList->head = NULL;
   theList->tail = NULL;
   theList->size = 0;
   *outList = theList;
}

Context

StackExchange Code Review Q#13007, answer score: 8

Revisions (0)

No revisions yet.