2
\$\begingroup\$

This is my second iteration of a function to remove all nodes containing value 'x'. The previous iteration is here.

This function takes as input the head of a list and has to delete all the nodes that contain a given value taken. Is this an improvement on the previous iteration? Is there anything else that can be improved?

typedef struct node {
    int val;
    struct node *next;
} node_t;

node_t *rimuovi(node_t *head, int x){

    node_t *temp = head;
    node_t *curr = head;

    if (head == NULL){
        printf("Lista vuota, finito!\n");
        return head;
    }

    while(head->val == x){
        if (head->next == NULL){
            free(head);
            printf("La lista adesso e' vuota. Finito\n");
            return head;
        }
        else {
            curr = head->next;
            free(head);
            head = curr;
        }
    }

    temp = head;
    curr = temp->next;

    while (curr != NULL){
        if (curr->val == x){
            if(curr->next != NULL){
                temp->next = curr->next;
                free (curr);
                curr = temp->next; 
            }
            else {
                temp->next = NULL;
            }
        }
        else {
            temp = curr;
            curr = curr->next;
        }
    }

    return head;
}
\$\endgroup\$
3
  • \$\begingroup\$ This looks like the code version before you modified it. \$\endgroup\$
    – forsvarir
    Commented Feb 11, 2020 at 15:20
  • \$\begingroup\$ yes cause they said me to post a new post not to modify the original one \$\endgroup\$
    – awwwww
    Commented Feb 11, 2020 at 16:06
  • 1
    \$\begingroup\$ The suggestion was that you ask a new question with the new version of your code. That way you can get feedback on the new version, without making the previous question confusing. I've edited your question to reflect a request for a followup review. If it doesn't match what you're looking for, please revert the change. \$\endgroup\$
    – forsvarir
    Commented Feb 11, 2020 at 20:03

2 Answers 2

2
\$\begingroup\$

Bug: Returning a free'd pointer!

After freeing a pointer, do not use it.

        free(head);
        printf("La lista adesso e' vuota. Finito\n");
        // return head;
        return NULL;

Is there anything else that can be improved?

Rather than 2 or 3 cases to handle an empty list, list of only matches or mixed list, consider a code simplification.

Create a temporary pre-head node and assign its next to the head. Only 1 case needed. This approach is reasonable when node_t is not large.

Some untested code.

node_t *rimuovi(node_t *head, int x) {
  node_t pre_head;
  node_t *p = &pre_head;
  pre_head.next = head;  // Other members are not used.

  while (p->next) {
    if (p->next->val == x) {
      node_t *next = p->next->next;
      free(p->next);
      p->next = next;
    } else {
      p = p->next;
    }
  }
  return pre_head.next;
}
\$\endgroup\$
0
\$\begingroup\$

Please include the necessary headers:

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

There's a lot of duplication between these branches:

while(head->val == x){
    if (head->next == NULL){
        free(head);
        printf("La lista adesso e' vuota. Finito\n");
        return head;
    }
    else {
        curr = head->next;
        free(head);
        head = curr;
    }
}

We always free(head), and we probably ought to set head = NULL in the first branch, rather than leaving it pointing to freed memory.
Therefore:

while (head->val == x) {
    curr = head->next;
    free(head);
    head = curr;
    if (curr == NULL){
        printf("La lista adesso e' vuota. Finito\n");
        return head;
    }
}

As chux says, use of a dummy head node can help us merge this with the following (non-head) logic.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.