Deleting a node being pointed to in a linked list

Question

This functions receives the linked list as a parameter and has to delete the node of the list which element DNI coincides with the string given to get_string and then return the list. At the moment the function works well if the node given is the first one but if the node given is any but the first it always removes the first element and messes up the list. What should I do to fix it?

PPACIENTE p_discharge(PPACIENTE pac){
  char dni[10];
  int u, c=0;
  PPACIENTE pAux;
  PPACIENTE temp;
  pAux=pac;
  fprintf(stdout,"Discharge\n\n");
  if (pac==NULL)
  {
    printf("No patients yet\n");
  }
  else
  {
    get_string("DNI",9,9,dni);
    while(pAux!=NULL){
      if(strcmp(pAux->DNI,dni)==0) {
        u=1;
        break;
      }
      else{
        pAux=pAux->sig;
      }
    }
    if (u==1){
      if (pAux->num==1)
      {
         pac=pAux->sig;
         free(pAux);
         return pac;
      }
      else 
      {
        printf("%d\n", pAux->num);
        c=pAux->num;
        while(pAux->num!=(c-2))
          pAux=pAux->sig;

        temp=pAux->sig;
        pAux->sig=temp->sig;
        free(temp);
      }
    }
    else{
      printf("Unknown patient\n");
    }
  }
  return pAux;
}

I'm not exactly sure what the role of `u` is, but it is not always assigned a value before it reaches the test `if(u==1)`. You should ensure it starts from a known value or always gets assigned before being tested. Also, you should try to rename it to something more descriptive of what the variable is for. — Outrageous Bacon, Jan 05 '21 at 17:30
I see now, @OutrageousBacon that `u`is redundant. Anyway, it only serves the purpose of checking whether or not the condition on the `strcmp()` is true, so at the start it always should be 0. I don't think that's the problem though. — Serxio García, Jan 05 '21 at 17:39
I do not understand you use of field *num* you use to do `if (pAux->num==1)` but when you remove a cell you never update that field for the next cells — bruno, Jan 05 '21 at 17:43
I don't know what you are trying to do with the `pAux->num` checks. — Ian Abbott, Jan 05 '21 at 17:44
`has to delete the node of the list which element DNI coincides with the string given to get_string` : this is not what you do, all depends on the value of `pAux->num` — bruno, Jan 05 '21 at 17:53
@bruno right, could I change it to `if (pAux==pac)` to check if it is the first element and not use that field that I would have to change in every other node? — Serxio García, Jan 05 '21 at 17:54
@SerxioGarcía that seems logic, note this is what is proposed in the answer (I deleted mine) — bruno, Jan 05 '21 at 17:55
If `num` is supposed to be which element of the list it is, then removing an element requires reducing the value of `num` for all elements after it. If that's not what `num` is, the `while` condition doesn't make much sense. — David Schwartz, Jan 05 '21 at 18:00

score 0 · Accepted Answer · answered Jan 05 '21 at 17:53

I do not understand the purpose of the num member, so I will ignore it here.

In order to delete an element of the list, you need a pointer to the previous element, if any, so that the previous element's next link (called sig in your code can be updated.

PPACIENTE p_discharge(PPACIENTE pac){
  char dni[10];
  int u = 0;
  PPACIENTE pAux;
  PPACIENTE temp = NULL;  // Pointer to previous element of list
  pAux=pac;
  fprintf(stdout,"Discharge\n\n");
  if (pac==NULL)
  {
    printf("No patients yet\n");
  }
  else
  {
    get_string("DNI",9,9,dni);
    while(pAux!=NULL){
      if(strcmp(pAux->DNI,dni)==0) {
        u=1;
        break;
      }
      else{
        temp=pAux;
        pAux=pAux->sig;
      }
    }
    if (u==1){
      if (pAux==pac)
      {
         // found patient is first on the list
         // update head of list
         pac=pAux->sig;
      }
      else
      {
         // found patient is not the first on the list
         // update link in previous element
         temp->sig=pAux->sig;
      }
      free(pAux);
    }
    else{
      printf("Unknown patient\n");
    }
  }
  return pac;
}

This solved the problem, I was using `num` wrongly, so using `if (pAux==pac)` to check the first element on the list is much more logical. Thanks! — Serxio García, Jan 05 '21 at 18:02

Matteo Pinna · Answer 2 · 2021-01-05T18:04:18.987

In your code you're not properly handling the cases where you remove a middle or last element of the list. Try something like this, use a prec element that tells you if you're deleting the first element (prec=NULL) or not (prec!=NULL).

PPACIENT prec = NULL, pAux;
pAux = pac;
if (pac==NULL) {
    printf("No patients yet\n");
}
else {
    get_string("DNI",9,9,dni);
    while (pAux!=NULL) {
        if (strcmp(pAux->DNI, dni)==0) {
            if (prec==NULL) {// deleting first node
                pac = pAux->sig;
                pAux->sig = NULL;
                free(pAux);
                return pac;
            }
            else if (pAux->next==NULL) { // deleting last node
                prec->sig = NULL;
                pAux->sig = NULL;
                free(pAux);
                return pac;
            }
            else {  // deleting a middle node
                prec->sig = pAux->sig;
                pAux->sig = NULL;
                free(pAux);
                return pac;
            }
        }
        pAux = pAux->sig;
    }
}
printf("Unknown patient\n");
return pac;

Deleting a node being pointed to in a linked list

2 Answers2