Error in my Code(Deletion(Linked Lists))

[XrossHAIR]

The Number is not deleting(Pls.Help)

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


typedef struct con {
    int num;
    struct con *next;
} node;


void take(node *list)
{
    printf("\n\tEnter the Number(0 to END): ");
    scanf("%d",&(*list).num);
    if(list->num!=0)
            {
                list->next = (node *)malloc(sizeof(node));
                take(list->next);
            }
}


void printfrom(node *list)
{
    printf("%d",list->num);
    if(list->next->num!=0)
    {
        printf("->");
        printfrom(list->next);
    }
}


void del(node *list,int a)
{


    if((*list).num==a)
       {
            list = (*list).next;
       }
       else del((*list).next,a);
}


int main()
{
    printf("\n\t\t\tDeletion(Linked Lists)\n");
    node *head;
    int a;
    head = (node *)malloc(sizeof(node));
    take(head);
    printfrom(head);
    printf("\n\tEnter the Number you want to remove: ");
    scanf("%d",&a);
    del(head,a);
    printfrom(head);
    return 0;
}

[hamster_nz]

The pattern followed seems a little bit off.

I would expect to only allocate a new node when you have a non-zero number.

But for deleting, you have three cases.

1. The item is the head of the list.

2. The item is in the middle of the list.

3. The item is not found.

I can't see how you have all cases covered.

Also, recursion (e.g. in printfrom()) is not the best way to handle linked lists.

[hamster_nz]

There is a bigger logical problem - what does an empty list look like?

[hamster_nz]

Oh, and if you call malloc() to allocate something, you should also be calling free() to release the memory back to the system.

[XrossHAIR]

I want to know what is wrong in the above code.. bro

[XrossHAIR]

I want to know what is wrong in the above code.. bro

[stahta01]

Casting malloc - Cprogramming.com

The code is crap; throw it out and start over or use the advise and fix it!

Asking over and over what is wrong is a waste of time!

[XrossHAIR]

Sorry to ask this again, but I am not getting it. I am unable to figure out what is wrong.. I am following a book: advanced topics in c by noel kalicharan. If I just let this go, then I will not learn what's the mistake. I know I am skipping some parts. I want to know what will happen now. Please Kindly grant my request. - XrossHAIR

[stahta01]

Find another book because you have no idea what you are doing!

If I tell you to remove the Ace of Spades from a deck?
What card would you remove?

Edit: Please do not ever PM again!! If you do I will ask that you be banned!

Tim S.

[laserlight]

hamster_nz went over a bunch of your mistakes, but you didn't seem inclined to go through them.

[hamster_nz]

Ok... let's see.

Good points.

It compiles without errors.

Bad points - just about everything else.

So let's start with the printfrom() function.

void printfrom(node *list)
{
  printf("%d",list->num);
  if(list->next->num!=0)
  {
    printf("->");
    printfrom(list->next);
  }
}

What happens you try and print an empty list? It crashes.

Why is it made recursive? With enough data in the list it too may crash the program! (please - nobody point out that tail recursion optimization can stop this)

So let's fix that one . Here's the standard pattern for walking a linked list

void process_list(node *head) {
   node *current = head;
   while(current != NULL) {
      /* do something with the current node - in your case print it */
      current = current->next;
   }
}

It won't crash, and it will work with any length of list.

Note: You can always see if you need to print a separator or not - if (current == head) then you are on the first item so no separator is needed.

If you can change your printfrom() to something more like that, then show me your working and we will move on to how 'take()' is wrong.

[XrossHAIR]

Here, I changed it.. @hamster_nz

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


typedef struct con {
    int num;
    struct con *next;
} node;


void take(node *list)
{
    node *temp = list;
                printf("\n\tEnter the Number(0 to END): ");
                scanf("%d",&(*temp).num);
    while(temp->num!=0)
            {
                    temp->next = (node *)malloc(sizeof(node));
                    temp = temp->next;
                 printf("\n\tEnter the Number(0 to END): ");
                scanf("%d",&(*temp).num);
            }
        temp->next = NULL;
}


void printfrom(node *list)
{
    node *temp;
    temp = list;
    while(temp->next!=NULL)
        {
            printf("%d",temp->num);
            if(temp->next->next!=NULL)
                        printf("->");


                    temp = temp->next;
        }
}


void del(node *list,int a)
{
    node *temp;
if(list->num==a)
        {temp  = (*list).next;
            free(list);
            list = temp;
}


   else
        del(list->next,a);
}


int main()
{
    printf("\n\t\t\tDeletion(Linked Lists)\n");
    node *head;
    int a;
    head = (node *)malloc(sizeof(node));
    take(head);
    printfrom(head);
    printf("\n\tEnter the Number you want to remove: ");
    scanf("%d",&a);
    del(head,a);
    printf("\n\n\t\t");
    printfrom(head);
    return 0;
}

EDIT : I changed take() too. I hope it is the correct way.

[hamster_nz]

Looking good!

Do you really want to add the zero to the list?

[XrossHAIR]

Because I don't know how to end the take() without using it(ZERO).

EDIT: should I free the last one(ZERO) and put previous pointer to null?

[hamster_nz]

I would do it another way... get the input into an int variable, and then only malloc and add the node when it is != 0.

void take(node **list_end)
{
    while(1) {
        int a;
        node *new_node;

        // Get the user input
        printf("\n\tEnter the Number(0 to END): ");
        if(scanf("%d", &a) != 1)
            break;  // Input failed
        if(a == 0)
            break; // User entered a zero, or maybe bad input

        // Create a new node
        new_node = malloc(sizeof(node));
        if(!new_node)
            break;
        new_node->num = a;
        new_node->next = NULL;

        // Add it to the list
        *list_end = new_node;
        list_end = &(new_node->next);
    }
}

If you don't mind the list being in reverse order (older at the end) it gives much clearer and cleaner code.

void take(node **head)
{
    while(1) {
        int a;
        node *new_node;

        // Get the user input
        printf("\n\tEnter the Number(0 to END): ");
        if(scanf("%d", &a) != 1)
            break;  // Input failed
        if(a == 0)
            break; // User entered a zero, or maybe bad input

        // Create a new node
        new_node = malloc(sizeof(node));
        if(!new_node)
            break;
        new_node->num = a;

        // Add it to the start of the list
        new_node->next = *head; // Next is current head.
        *head = new_node;   // Head is now the new node.
    }
}