1

I came across this site, where removing last node of a linked list is implemented with the following code:

int remove_last(node_t * head) {
    int retval = 0;
    /* if there is only one item in the list, remove it */
    if (head->next == NULL) {
        head->val
        free(head);
        head = NULL;
        return retval;
    }

    node_t * current = head;

    while (current->next->next != NULL) {
        current = current->next;
    }
}

My question is simple, isn't this example wrong? I mean it will free the memory of the node only if there is only one node. In any other case it does not really do something, only keeps going to the next node in while (current->next->next != NULL) loop. Am I not noticing something?

3
  • 2
    It would seem like someone forgot to finish writing the function - there's no return statement either... Commented Aug 12, 2014 at 18:37
  • 1
    You are right : it is wrong.
    – francis
    Commented Aug 12, 2014 at 18:38
  • 1
    head->val, head = NULL; : no effect.
    – BLUEPIXY
    Commented Aug 12, 2014 at 18:46

2 Answers 2

1

You are right. The method is wrong. Moreover head itself can be equal to NULL so even the first executable statement of the method

if (head->next == NULL) {

can result in program abend.

And below this statement there is a syntax error

if (head->next == NULL) {
    head->val
    free(head);

So the code will not be compiled.

0
1

you can try this one:

int remove_last(node_t * head)
{
    int retval = 0;
    /* if there is only one item in the list, remove it */
    if (head->next == NULL) 
    {
        retval = head->val;
        free(head);
        head = NULL;
        return retval;
    }
    /*if there are another items */
    else
    {
        node_t * current = head;
        while (current->next->next != NULL) 
            current = current->next;
        retval = current->val;
        free(current->next);
        current->next = NULL;
    }
    return retval;
}
2
  • 1
    This code is better but only partially correct. The return value is inconsistent: In the if block retval is the value of the last node (head), and in the else block retval is the value of the node that becomes the last node (current). also the return statement in the if block is not neccessary and head = NULL has no effect at all. Commented Aug 12, 2014 at 18:56
  • That's correct, I prefer to return the list though or nothing.
    – Nanc
    Commented Aug 12, 2014 at 19:00

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.