In linked list ,when i write (temp->next=newnode) then when i apply temp=temp->next does not work but temp=newnode works?

Question

Below is my code:

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

int main() {
    struct node {
        int data;
        struct node *next;
    };
    int choice;
    struct node *head, *newnode, *temp;
    head = 0;
    while (choice) {
        newnode = (struct node *)malloc(sizeof(struct node));
        printf("enter items ");
        scanf("%d", &newnode->data);
        newnode->next = 0;
        if (head == 0) {
            head = temp = newnode;
        } else
            temp->next = newnode;  /** **this is the problem** **/
            temp = newnode;      /** temp=newnode works but temp=temp->next doesn't**/
        printf("do you want to continue");
        scanf("%d", &choice);
    }
    temp = head;
    while (temp != 0) {
        printf("list is %d \n", temp->data);
        temp = temp->next;
    }
}

Shouldn't there be curly braces around the two lines following the `else` statement? — r3mainer, Feb 14 '20 at 05:20
the problem get solved after curly braces; but the list was working even before that without giving any error, can you please explain why that happened? — user12552749, Feb 14 '20 at 05:52
@user12552749 If you can provide the difference in output or behavior of the list before adding the curly braces, maybe I can help with the explanation — Akash Das, Feb 14 '20 at 07:37
Side note: That your code does anything at all is pure accident, you haven't initialised `choice`, if it by accident gets 0, then you won't enter your loop at all. You should either initialise it to something != 0 – or convert your loop into a do-while, then `choice` won't be read before having been assigned within `scanf`. — Aconcagua, Feb 14 '20 at 09:06
Side note 2: The cast after malloc is not necessary in C (but *is* in C++). Should you still do so? Oppinions [vary](https://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc), best read both sides and make up your own mind... — Aconcagua, Feb 14 '20 at 09:11
Recommendation: *Always* put braces around the body of if blocks or loops, even if you do not need (many coding guidelines mandate as well, most notably MISRA). This will avoid forgetting to place them if you extend the line later, more important, though, it will prevent errors in case of a seeming function actually being a (badly defined) multi-expression macro (something like `#define f() g(); h();`). If you don't want to follow, then maybe at least this one: If you have braces in *any* of the different branches, then place them around *all* of them. — Aconcagua, Feb 14 '20 at 09:14
@Aconcagua a garbage value would have been assigned to choice — user12552749, Feb 14 '20 at 09:23
@user12552749 Sure. But there's no guarantee that this garbage cannot be, by accident, 0 as well, so don't rely on. In any case, reading uninitialised variables is undefined behaviour. And actually, it is not *assigned*, it's just the value that happened to be at the memory location where `choice` is placed at. — Aconcagua, Feb 14 '20 at 09:26
About the missing curly braces (see @r3mainer): You want to assign `newnode` to `temp` in both branches anyway. So doing the assignment of the else-branch actually outside of doesn't introduce a functional misbehaviour *in this very concrete case* (in case of `head` still being 0, you just do the assignment twice) – your indentations reveal, though, that you actually intended differently, so pure luck again, and from sight of code, one would still consider it as a bug, just without effects, so a hidden/invisible one (that might get effective any time on later code changes, though!)... — Aconcagua, Feb 14 '20 at 09:34

score 0 · Answer 1 · answered Feb 20 '20 at 09:03

The problem is here:

    if(head==0)
    {
        head=temp=newnode;
    }
    else
        temp->next=newnode;  /** **this is the problem** **/
        temp=newnode;      /** temp=newnode works but temp=temp->next doesn't**/

Indentation does not determine structure. You must enclose multiple instructions in a block to group them in the else clause. As coded, temp=newnode; is executed unconditionally after the test, which is not a problem, but redundant with head=temp=newnode.

Note also that choice is uninitialized when tested the first time.

Here is a corrected version:

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

int main() {
    struct node {
        int data;
        struct node *next;
    };
    int choice;
    struct node *head, *tail, *newnode, *temp;
    head = tail = NULL;
    for (;;) {
        newnode = (struct node *)malloc(sizeof(struct node));
        if (newnode == NULL)
            break;
        printf("enter item: ");
        if (scanf("%d", &newnode->data) != 1) {
            free(nownode);
            break;
        }
        newnode->next = NULL;
        if (head == NULL) {
            head = newnode;
        } else {
            tail->next = newnode;
        }
        tail = newnode;
        printf("do you want to continue? [0/1] ");
        if (scanf("%d", &choice) != 1 || choice == 0)
            break;
    }
    for (temp = head; temp != NULL; temp = temp->next) {
        printf("list is %d \n", temp->data);
    }
    return NULL;
}

In linked list ,when i write (temp->next=newnode) then when i apply temp=temp->next does not work but temp=newnode works?

1 Answers1