Inserting into a linked list always returns last node, not current node

Question

So I'm writing a program that creates a dinamic list, it inserts a new node after the last node and eventually it prints the freshly created list. I want that when I print it it actually prints the number of the current node in succession (like Info: 1, Info: 2, Info: 3) but it turns out it prints always the last node. I guess it's a pointer error, but can't really find what.

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

struct node{
  int info;
  struct node* pNext;
};

typedef struct node Node;

void tail_insertion(Node* pFirst, Node* pLast);
void print_list(Node* pFirst);

static int n=3;

int main()
{
    Node *pFirst=NULL, *pLast=NULL;
    tail_insertion(pFirst, pLast);
}

void tail_insertion(Node* pFirst, Node* pLast){
  // Creation of a new node in the heap
  Node *pNew = (Node*) malloc(sizeof(Node));
  pNew->info=0;
  pNew->pNext= NULL;

  for(int i=0; i<3;i++){
    if(pFirst == NULL){// No node in the list
      pNew->info++;
      pFirst = pNew; // The first node is the newly created one
      pLast = pNew; // The last node is the newly created one
      printf("Ok the list was empty\n");
    }
    else{
      // Else, there is already at least one node in the list
      pNew->info++;
      pLast-> pNext= pNew; // the last node becomes the second one
      pLast= pNew; // The last node is the newly created one
      printf("Ok the list wasn't empty\n");
    }
  }
  print_list(pFirst);
  return;
}


void print_list(Node* pFirst){
  if(pFirst == NULL){
    printf("No node in the list!\n");
  }
  else{
    // New pointer used to scan the list.
    Node* pScan = pFirst;
    do{
      printf("Info: %d\n", pScan->info);
      // ptrScan is updated to point to the next node in the
      // list
      pScan = pScan->pNext;
    }while(pScan!= NULL && --n); //NULL when this was the last node
  }
  return;
}

It prints so:

Ok the list was empty
Ok the list wasn't empty
Ok the list wasn't empty
Info: 3
Info: 3
Info: 3

Answer 1

Code fix

Using you own code, you should fix tail_insertion(...) to be like the code below. There are comments to help understand the changes.

Node *pNew; /* Declaring pNew *without* assignment. */

for(int i = 0; i < 3; i++){
    
    pNew = (Node*) malloc(sizeof(Node)); /* Allocation of new memory address. */
    
    pNew->info  = i; /* pNew->info works like an ID in you current code. */

    pNew->pNext = NULL; /* It is proper to assign NULL */
    
    if(pFirst == NULL){
        
        pNew->info++;
        
        pFirst       = pNew;
        pLast        = pNew;
        
        printf("The list was empty.\n");
        
    } else {
        
        pNew->info++;
        
        pLast->pNext = pNew;
        pLast        = pNew;
        
        printf("The list wasn't empty.\n");
        
    }
}

print_list(pFirst);

Note that I have removed the return(...) function since it wasn't necessary. It is not necessary because tail_insertion(...) has the type void.

Also, pNew->info is storing the ID of the node as of my reading. So it shouldn't be a problem assigning i to it. If necessary, make the proper changes.

Now.. Let's begin explaining things from the beginning!

General Code Analysis

Let's consider tail_insertion(...) and what you want it to do:

• You create a new node;
• You give it the right values;
• You start a for loop with the intention of creating more nodes based on certain conditions;
• You print the whole list to check if it worked;

Now, look at your code for tail_insertion(...) function (comments removed):

Node *pNew = (Node*) malloc(sizeof(Node));

pNew->info=0;
pNew->pNext= NULL;

for(int i=0; i<3;i++){
    
    if(pFirst == NULL){
        
        pNew->info++;

        pFirst = pNew;
        pLast = pNew;
        
        printf("Ok, the list was empty.\n");
        
    } else {
        
        pNew->info++;

        pLast-> pNext= pNew;
        pLast= pNew;
        
        printf("Ok, the list wasn't empty.\n");
        
    }
    
}

print_list(pFirst);

Seeing any problem yet? No? Well, let's proceed.

Debugging

We are going to debug this function step-by-step. Aside from program fixes, it is important for the learning process as well:

• Creating a node:
  • You create pNEW. It is empty;
  • Let's give it the address 0x01 (fictious);
  • A 0 is assigned to pNew->info;
  • A NULL is assigned to pNew->pNext;
• Beginning of the for loop - an attempt to create 3 nodes for a list:
  • i = 0:
    • You check to see if the list is empty. It is:
      • You add 1 to pNew->info value. The address of pNew is 0x01;
      • pFirst is assigned to point at the address 0x01;
      • pLast is assigned to point at the address 0x01 since the list was empty;
      • You print a message saying the list was empty;
  • At the end of i = 0, the list is as follows:
    • pFirst points at the address 0x01;
    • pLast points at the address 0x01;
    • 0x01->info is 1;
  • i = 1
    • You check to see if the list is empty. It is not:
      • You add 1 to pNew->info value. The address of pNew is 0x01. There was never a creation of another node;
      • pLast->pNext is assigned to point at the address 0x01;
      • pLast is assigned to point at the address 0x01;
      • You print a message saying the list wasn't empty;
  • At the end of i = 1, the list is as follows:
    • pFirst points at the address 0x01;
    • pLast points at the address 0x01;
    • pLast->pNext points at the address 0x01;
    • 0x01->info is 2;
  • i = 2
    • You check to see if the list is empty. It is not:
      • You add 1 to pNew->info value. The address of pNew is 0x01. There was never a creation of another node;
      • pLast->pNext is assigned to point at the address 0x01;
      • pLast is assigned to point at the address 0x01;
      • You print a message saying the list wasn't empty;
  • At the end of i = 2, the list is as follows:
    • pFirst points at the address 0x01;
    • pLast points at the address 0x01;
    • pLast->pNext points at the address 0x01;
    • pLast->pNext->pNext points at the address 0x01;
    • 0x01->info is 3;
  • End of the for loop;
• Printing the resulting list:
  • A call for print_list(...)

Conclusion

As of now, you probably saw what's wrong: you haven't created a new node inside the for loop. You created it outside of the loop. This makes a repetitive process of adding the very same node with the same address to the list.

Also, you are having problems with the value of info. But that is thanks to using the same node as stated above. It will need just some adjusting in this incremental logic that sets info value to accommodate the modifications need to fix the usage of the same node and make everything work.

Final Notes

There are comments saying you don't need pLast. While that is true, you need to consider:

• If you only use pFirst, you need to make sure that after inserting the new node in pFirst->pNext, you do a pFirst->pNext->pNext = NULL. This way you can know whenever you have reached the end of the list by comparing to NULL;
• If you continue to use pLast, you can compare to pLast to know if the address is the last address inside the list. Obviously, you can also compare to NULL after my fix;
• Considering tail insertions, having pLast facilitates things quite a bit and reduces CPU usage in the long run;

You probably know but you should put print_list(pFirst); inside the main(...) function to makes things more organized. Also, it makes my "code OCD" reach new levels of desperation! ;-)

Answer 2

Two problems:

• you are not passing in pointers to the list pointers (Node * -> Node **)
• you are trying to insert the same Node three times (move malloc() into the loop)

I guess static int n = 3 and while ... && --n) have probably been added in a desperate attempt to "fix" the endless loop in print_list(), because pscan->pNext yielded pscan in the original code...

Fixed and cleaned up code:

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

typedef struct node Node;
typedef struct node {
  int   info;
  Node* pNext;
} Node;

static void print_list(Node* pFirst) {
  // New pointer used to scan the list.
  Node* pScan = pFirst;
  while (pScan) {
    printf("Info: %d\n", pScan->info);
    // ptrScan is updated to point to the next node in the
    // list
    pScan = pScan->pNext;
  }
}

static void tail_insertion(Node** pFirst, Node** pLast) {
  for (int i=0; i<3; i++) {
    // Creation of a new node in the heap
    Node *pNew  = malloc(sizeof(Node));
    pNew->info  = i + 1;
    pNew->pNext = NULL;

    if (*pFirst == NULL) {    // No node in the list
      *pFirst = pNew;         // The first node is the newly created one
      *pLast  = pNew;         // The last node is the newly created one
      printf("Ok the list was empty\n");
    } else {                  // Else, there is already at least one node in the list
      (*pLast)->pNext = pNew; // the last node becomes the second one
      *pLast          = pNew; // The last node is the newly created one
      printf("Ok the list wasn't empty\n");
    }
  }
  print_list(*pFirst);
}

int main(int argc, char *argv[]) {
  Node *pFirst = NULL;
  Node *pLast  = NULL;
  tail_insertion(&pFirst, &pLast);
}

Test run:

$ gcc -Wall -Werror -o dummy dummy.c
$ ./dummy
Ok the list was empty
Ok the list wasn't empty
Ok the list wasn't empty
Info: 1
Info: 2
Info: 3