Appending node to linked list

Question

So This code was working fine when I was inserting the node towards the NULL cell. I tried implementing this to send the cells to the beginning, but since then the display_list function only shows the last cell. I've been trying to figure it out for a while. Suggestions?

I will add that this is supposed to be a function that mimics dc in Linux.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

struct CELL {
  int val;
  struct CELL *next;
};

void append_node(struct CELL *llist, int num);
void display_list(struct CELL *llist);

The main seems to be fine

int main(void)
{

  int num = 0;
  int first = 0;
  int input = 0;
  char quit = 'n';
  char inputchar = ' ';

  struct CELL *llist;

  llist = (struct CELL *)malloc(sizeof(struct CELL));
  llist->next = NULL;

  while (quit == 'n'){

    if (scanf("%d", &input) == 1){

      if ( first == 1 )
        append_node(llist, input);


      if ( first == 0){
        llist->val = input;
        first = 1;
      }
    }

    else{
      inputchar = getchar();

      if (llist->next == NULL && first == 0)
        printf("List is empty.\n");

      if (inputchar == 'f')
        display_list(llist);

      if (inputchar == 'q')
        quit = 'y';
      else if (llist->next != NULL){
        switch (inputchar){

        case 'q':
        quit = 'y';
        break;
    }
      }
    }


  }
  free(llist);
  return 0;
}

The commented out code was working well! This was until I discovered I was supposed to be adding the cells to the other end, which I'm having difficulty figuring out. What am I missing here?

void append_node(struct CELL *llist, int num) {
  /* while(llist->next != NULL)
     llist = llist->next;
  llist->next = (struct CELL *)malloc(sizeof(struct CELL));
  llist->next->val = num;
  llist->next->next = NULL;*/

  struct CELL *temp;
  temp = (struct CELL *)malloc(sizeof(struct CELL));
  temp->val = num;
  temp->next = llist;
  llist = temp;
}

void display_list(struct CELL *llist)
{
  while(llist->next != NULL) {
    printf("%d\n", llist->val);
    llist = llist->next;
  }
  printf("%d\n", llist->val);
}

I will admit that I have difficulty knowing when I should use pointers, and I suspect that I may be missing one somewhere. Any help would be appreciated.

Answer 1

Look at this part of your code,

void append_node(struct CELL *llist, int num) {
  struct CELL *temp;
  temp = (struct CELL *)malloc(sizeof(struct CELL));
  temp->val = num;
  temp->next = llist;
  llist = temp;   // Line1
}

Notice Line1 : When you change llist to point to the new node you are changing the local copy of llist while llist in main continues to retain its old value.

How do I correct this ?

This is a flaw in your design of the linked list. The client program(main) should not access the CELL structure at all. You should have another structure which represents the linked list and has a pointer to the first cell.

Something like this,

struct LinkedList {
  struct CELL *head;
};

Your main should be using this structure and not CELL.

---

Few other thing I saw in your code,

1) display_list function will fail if NULL is passed to it. It could be better done like this,

void display_list(struct CELL *llist)
{
  while(llist != NULL) {
    printf("%d\n", llist->val);
    llist = llist->next;
  }
}

2) See this line at the end of your main,

free(llist);

You are freeing ONLY the first cell in the linked list. You have not freed the other cells which were added to the list. This will cause a Memory leak in your program.

How do I solve this ? Freeing the linked list should not be done by the client(main) code. You should provide another function which will recursively free all the allocated cells. Again, this is much easier if you follow the above suggested design with a structure representing the linked list.

---

Edit : Added an example on request in the comments section.

Your display will look something like this if you change your design to what I have suggested,

void display_list(struct LinkedList *llist)
{
  struct CELL * head = llist->head;
  while(head != NULL) {
    printf("%d\n", head->val);
    head = head->next;
  }
}

Answer 2

@Mohammad Ghazanfar points are true and one should take care of those points. On the other hand you can change the following functions to make your code working.

changing function signature of void append_node(struct CELL *llist, int num); to void append_node(struct CELL **llist, int num); and the function defination is as follows

void append_node(struct CELL **llist, int num) {
  /* while(llist->next != NULL)
     llist = llist->next;
  llist->next = (struct CELL *)malloc(sizeof(struct CELL));
  llist->next->val = num;
  llist->next->next = NULL;*/

  struct CELL *temp;
  temp = (struct CELL *)malloc(sizeof(struct CELL));
  temp->val = num;
  temp->next = (*llist);
  (*llist) = temp;
  return;
}

and replacing the call of append_node(llist, input); as append_node(&llist, input);

Note :- I have just made your code working. This might not be the perfect solution. You should consider points mentioned by @Mohammad Ghazanfar.

Hope this helps :)