0
struct node{
    int elem ;
    node* next;
};
typedef node* list;

void RemoveAllX(int x, list&l){
    list copy = l;
    list q;
    while (l != NULL){
        if (l -> elem == x){
            q = l;
            l = l -> next;
            delete q;
        }
        else
            l = l -> next;
    }
    l = copy;
}

What I'm trying to do is remove from the list every node that has an integer x as an element. I would like that if I have a list let's say [4]->[1]->[1]->[3] and made the call RemoveAll(1, mylist) I get the list [4]->[3], however what I'm getting with this code is [4]->[trash]->[trash]->[3]. I'm assuming the line l = l -> next; inside the if statement isn't working as I understood it should, any help appreciated

Matías Santurio
  • 189
  • 2
  • 9
  • 1
    You never modify the structure of the list. If you want remove a node, you need to "route around" it, so you must assign to a `next` member at some point. Drawing boxes and arrows with pen and paper is the most efficient way of figuring out pointer-manipulating code. – molbdnilo Apr 05 '20 at 06:43
  • 1
    Draw pictures. Seriously. The best way to get a grip on a linked list is to visualize the sucker. Draw the list. then step by step sever the links holding the node you want gone into the list and attach the two nodes around it. When you are done you have a bunch of pictures that form the basis of your code AND all of the expectations you need to debug the code if it doesn't work. – user4581301 Apr 05 '20 at 06:45
  • 1
    Hey thanks for your answer, that's actually what I've been doing (drawing boxes and arrows). But I think I see what you mean, by doing what I show in that code, I'm just going through the list with the variable "l" I'm not modifying the real thing, just moving a pointer around. Did I get right? – Matías Santurio Apr 05 '20 at 06:48
  • I recommend taking a step back doing some earlier exercises. Make sure you know the differences between values pointers and references and how and when to use them. – user4581301 Apr 05 '20 at 06:56
  • Once you [grok](https://en.wikipedia.org/wiki/Grok) pointers, take a look at the [community addition in this answer](https://stackoverflow.com/a/22122095/4581301). – user4581301 Apr 05 '20 at 07:00
  • 1
    @S.M. C doesn't have a `delete` This code does. Kinda makes a mess of things, doesn't it? – user4581301 Apr 05 '20 at 07:01
  • Thanks for your feedback @user4581301, but what do you mean ` l = l -> next` is fantasy code? That's what I've been using to move around the list. Also I defined ´list´ as ´node*´ so I can treat them equally right? – Matías Santurio Apr 05 '20 at 07:03
  • @MatíasSanturio I just finally spotted the `typedef`. I have a bit of a blind spot for them. Anyway, that's the *weird smurf elsewhere in your program* I was talking about. Now that I've made that connection, the code makes more sense. – user4581301 Apr 05 '20 at 07:11
  • Quick question: What's the point of the copy? If you remove all the nodes, that copy's next pointer is going to be invalid. – user4581301 Apr 05 '20 at 07:22
  • I edited the question a little bit, hope it's clearer on what I wanted the code to do. – Matías Santurio Apr 05 '20 at 07:32

2 Answers2

0
  1. this is "sort of" code to delete a single node. Your method says RemoveAll
  2. second you are comparing an int to a pointer reference
  3. you need yo make sure that you are connecting the rest of the linked list back to the prev node of the one you deleted. You could lose the rest of the list if you dont.
void deleteNode(Node *head, Node *n)  
{  
    // When node to be deleted is head node  
    if(head == n)  
    {  
        if(head->next == NULL)  
        {  
            cout << "There is only one node." << 
                    " The list can't be made empty ";  
            return;  
        }  

        /* Copy the data of next node to head */
        head->data = head->next->data;  

        // store address of next node  
        n = head->next;  

        // Remove the link of next node  
        head->next = head->next->next;  //this is the line you want... it connect the rest

        // free memory  
        delete n;  //can use free() as well , delete will call 
                   //deconstructor free wont...

        return;  
    }

this is from geekstogeeks

jeremygblake
  • 61
  • 6
0

So thanks to the help left in the comments I was able to come up with this working but probably bad looking code.

struct node{
    int elem ;
    node* next;
};
typedef node* list;

void RemoveAllX(int x, list &l){
    if (l != NULL){
        list p = l; // Just a copy I'll be using, so I don't mess the original list up
        list aux; // auxiliary variable I'll use to make the deletion.
        while (p -> next != NULL){ // Here I look  for the any x's in every node BUT the first
            if ((p -> next) -> elem == x){
                aux = p -> next;
                p -> next = (p -> next) -> next; // "Skip" or "go around" the node I want to remove 
                delete aux; 
            }
            else{
                p = p -> next;
            }
        }
        if (l -> elem == x){ // And finally I check the first node the one I "missed" before
            aux = l;
            l = l -> next;
            delete(aux);
        }
    }
}
Matías Santurio
  • 189
  • 2
  • 9