Generic linked list code C

Question 1

I have created this code: implementation of a linked list in C.

To test it I created a label struct which contains astring andint variables.

The code is commented well and tested before uploaded to here.

I would like to hear your opinion about it, also I'm not that sure thefree andmalloc are working well (I usedDrMemory and it showed no mistakes but memory is a "leaky" thing (lol)).

#include <stdio.h>#include <string.h>#include <stdlib.h>/*label *//*The label is managed by lnode, which contains the address-string(8-bits)  and the label-name. */  typedef struct label{            int   address;             /* The address number.                      */            char* name;                /* The name of the label.                   */  }label;/*Label implementation*//*Create label object*/  label* createLabel(int address, char* name)  {      int len;      label* newlabel;      if(NULL == name){         return NULL;      }      len = strlen(name)+1;      newlabel = (label*)malloc(sizeof(label));      if(NULL == (newlabel->name = (char*)malloc(sizeof(char)*len))){        return NULL;      }    /*copy name to new-label name*/      strcpy(newlabel->name, name);    /*set address in newlabel*/      newlabel->address = address;      return(newlabel);  }/*delete label object*/  void deleteLabel(void* ob ){       label *lbl;       if(NULL == ob){          return;       }       lbl = (label*)ob;     /*delete the name*/       free(lbl->name);     /*delete the Label object*/       free(lbl);  }/*Print Label-objects --> for Testing*//*Allocate a string and fill with label data*/  void printLabel(void* ob){       label* obl= (label*)ob;       printf("%s%d,  label: %-10s, ","address: ",obl->address,obl->name);  }/*gnode*//*generic-node struct to be used in a generic list*/  struct gnode{       void *value;        /*pointer to generic-node */       struct gnode* next; /*pointer to next node    */  }typedef gnode;/*createGnode*//*create a gnode to be added to a generic-list*/  gnode* createGnode( void* value, gnode* next){      gnode* newnode=NULL;      if(NULL == value){         return NULL;      }      if (NULL == (newnode = (gnode*)malloc(sizeof(gnode)))) {        return NULL;      }    /*add the value*/      newnode->value = value;    /*set next value to be NULL --> All added gnode are      added at the end of the list and become the last node */      newnode->next = next;      return(newnode);  }/*freeGnode*//*free the allocated gnode, using a function to delete the pointed value  */  void freeGnode(gnode* nodetodelete,  void (*deletefunc)( void*)){     if(NULL == nodetodelete){        return;     }   /*delete the value, pointed by void* value */     deletefunc(nodetodelete->value);   /*free the allocated gnode*/     free(nodetodelete);  }/*insertGnode --> insert gnode to list*/  int insertGnode(gnode** glist, gnode* newnode)  {      gnode* curr; /*pointer to current node in list*/      if(NULL == newnode){         return -1; /*no item to insert*/      }    /*Empty list*/      else if(NULL == *glist){              *glist = newnode;              return 1;      }    /*add newnode in last place in list*/    /*Get to last node*/      for( curr = *glist; curr->next != NULL; curr = curr->next);    /*Add the newnode as last node*/      curr->next = newnode;    /*return Success*/      return 1;  }/*deleteGenericList*//*Parse each generic-node and delete */  void deleteGenericList(gnode* glist, void (*deleteValue)(void*))  {     gnode* curr=NULL;     gnode* next;     if(NULL == glist){        return;     }   /*Delete each generic-node*/     for(curr = glist; curr != NULL; ){         next = curr->next;         deleteValue(curr->value); /*free the value objext     */         free(curr);               /*free current generic-node */         curr = next;     }   /*Set glist to point at NULL*/     glist = NULL;  }/*End deleteGenericList*//*writeGlist --> write Gnode to an open-file*/  void writeGenericList(FILE* fp, gnode* glist, void (*writeValue)(FILE*, void*))  {      gnode* curr=NULL; /*curr node in the generic-list*/    /*check that all arguments are present*/      if( (NULL == fp)||(NULL==glist)||(NULL==writeValue) ){           return;      }    /*use the writeValue function to write the gnode into the file*/      for(curr = glist; curr != NULL; curr=curr->next){          writeValue(fp, curr->value);      }  }/*End writeGenericList*//*For testing...*/  void printGlist(gnode* glist, void (*printValue)(void*)){      gnode *curr;      if(NULL == glist){         return;      }      for (curr = glist; curr != NULL; curr = curr->next){           printValue(curr->value);      }  }  void printGlistOLD(gnode* glist, void (*printValue)(void*)){      gnode *curr;      if(NULL == glist){         return;      }      printf("%-20s\n", "GenericList: top-To-Bottom");      for (curr = glist; curr != NULL; curr = curr->next){           printValue(curr->value);      }  }  int main(int argc, char const *argv[])  {    printf("start\n");    label *label1 = createLabel(100,"hello world");    label *label2 = createLabel(101,"a");    label *label3 = createLabel(102,"b");    label *label4 = createLabel(100,"c");    label *label5 = createLabel(103,"d");    label *label6 = createLabel(102,"e");    gnode* g1 = createGnode(label1,NULL);    gnode* g2 = createGnode(label2,NULL);    gnode* g3 = createGnode(label3,NULL);    gnode* g4 = createGnode(label4,NULL);    gnode* g6 = createGnode(label6,NULL);    gnode* g5 = createGnode(label5,g6);    printf("%d\n",insertGnode(&g1,g2));    printf("%d\n",insertGnode(&g1,g3));    printf("%d\n",insertGnode(&g1,g4));    printf("%d\n",insertGnode(&g1,g5));    printGlistOLD(g1,printLabel);    deleteGenericList(g1,deleteLabel);    return 0;  }

Thanks!

Question 2

There's a memory leak in your code: if allocation of a new label succeeds but allocation for its name fails, youreturn NULL and do notfree() the label structure.

Question 3

As I mentioned in the previous review, don't duplicate code in comments. We understand the purpose ofstrcpy(newlabel->name, name); very well.
As I mentioned in the previous review,do not cast malloc.
insertGnode doesn'tinsert into, but ratherappends to a list. Consider renaming.
You don't seem to worry of the order of the nodes in the list. Considerprepending the new node, as in
```
int add_gnode(gnode** glist, gnode* newnode){    if (newnode == NULL) {        return -1;    }    newnode->next = *glist;    *glist = newnode;    return 1;}
```
This approach has two benefits:
- it doesn't traverse an entire list just to find the tail, and
- it doesn't special case an empty list.
printGlist shall callwriteGenericList with proper arguments.
You seem to strive toward OOP approach. Consider making agenericList structure havingdeletefunc,writeValue, etc as members.

Question 4

you consistently castvoid * when you should not in C language:

newlabel = (label*)malloc(sizeof(label));

should be:

newlabel = malloc(sizeof(label));

and

void printLabel(void* ob) {    label* obl = (label*)ob;

should be:

void printLabel(void* ob) {    label* obl = ob;

you have a potential memory leak increate_label:

newlabel = malloc(sizeof(label)); // don't cast mallocif (NULL == (newlabel->name = malloc(sizeof(char)*len))) {    return NULL;                  // leak previously allocated label}

names are inconsistent for gnode management:createGnode inserts node at first position when comment says last, andinsertGnode adds it at last position
freeGnode unconditionnaly frees a node and its content, possibly breaking any link if you remove a node in a list => show at least be explicit in a comment that you cannot remove a node from a list
indeleteGenericList(gnode* glist, void(*deleteValue)(void*)), the last instruction (glist = NULL;) is a no-op because is only set the local value in the function that will go out of scope at next line
you should considere to add a function to remove a node from a list

vnp 58.7k4 gold badges55 silver badges144 bronze badges · Answer 1 · 2017-08-21 06:48:45Z

As I mentioned in the previous review, don't duplicate code in comments. We understand the purpose ofstrcpy(newlabel->name, name); very well.
As I mentioned in the previous review,do not cast malloc.
insertGnode doesn'tinsert into, but ratherappends to a list. Consider renaming.
You don't seem to worry of the order of the nodes in the list. Considerprepending the new node, as in
```
int add_gnode(gnode** glist, gnode* newnode){    if (newnode == NULL) {        return -1;    }    newnode->next = *glist;    *glist = newnode;    return 1;}
```
This approach has two benefits:
- it doesn't traverse an entire list just to find the tail, and
- it doesn't special case an empty list.
printGlist shall callwriteGenericList with proper arguments.
You seem to strive toward OOP approach. Consider making agenericList structure havingdeletefunc,writeValue, etc as members.

Serge Ballesta 1,0399 silver badges18 bronze badges · Answer 2 · 2017-08-21 09:45:20Z

you consistently castvoid * when you should not in C language:

newlabel = (label*)malloc(sizeof(label));

should be:

newlabel = malloc(sizeof(label));

and

void printLabel(void* ob) {    label* obl = (label*)ob;

should be:

void printLabel(void* ob) {    label* obl = ob;

you have a potential memory leak increate_label:

newlabel = malloc(sizeof(label)); // don't cast mallocif (NULL == (newlabel->name = malloc(sizeof(char)*len))) {    return NULL;                  // leak previously allocated label}

names are inconsistent for gnode management:createGnode inserts node at first position when comment says last, andinsertGnode adds it at last position
freeGnode unconditionnaly frees a node and its content, possibly breaking any link if you remove a node in a list => show at least be explicit in a comment that you cannot remove a node from a list
indeleteGenericList(gnode* glist, void(*deleteValue)(void*)), the last instruction (glist = NULL;) is a no-op because is only set the local value in the function that will go out of scope at next line
you should considere to add a function to remove a node from a list

Movatterモバイル変換

Stack Exchange Network

Generic linked list code C

2 Answers2

You mustlog in to answer this question.

Related

Hot Network Questions

Subscribe to RSS