Insert linked list

Question

I am trying do a simple insert Node to last position of linked list. But I am running into trouble.

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

using namespace std;

struct Node
{
    int data;
    struct Node* next;
};

Node* Insert(Node* head, int data);
Node* print(Node* head);
void ReverseIterative();


Node* Insert(Node* head, int data)
{
    Node* newNode;
    newNode->data = data;
    newNode->next = NULL;

    if(head == NULL)
    {
        return newNode;
    }

    Node* curr=head;
    while(curr->next!=NULL)
    {
        curr=curr->next;
    }
    curr->next = newNode;
    return head;
}

Node* printList(Node* head)
{
    while(head)
    {
        cout<<head->data;
        head=head->next;
    }
    cout<<endl;
}

int main()
{
    struct Node* head = NULL;
    head = Insert(head, 2);
    head = Insert(head, 4);
    printList(head);
    return 0;
}

I am not sure what im doing wrong. Please help~! I looked over my logic and everything should be correct. It might be the way I am creating a new node in the insert(). I think there is something wrong with my syntax, but i am not sure what it is. I really hate when this happens...

Thanks for your help

"running into trouble" is not a very good description of your problem, is it? Please be **specific**. — OldProgrammer, Feb 22 '16 at 23:40
Why would you think it's your syntax? Are you getting a compiler error? If so, then what is it? — ClickRick, Feb 22 '16 at 23:41
Perfect time to stop looking at the code and step through it with a debugger. Pointer gonna point. One of yours doesn't. — user4581301, Feb 22 '16 at 23:41
The code is compilable, but when executing windows says "a.exe has stopped working" — Telenoobies, Feb 22 '16 at 23:43
Check the output warnings. Should be an uninitialized variable in there. — user4581301, Feb 22 '16 at 23:43
@Telenoobies _"The code is compilable,"_ That's the very wrong response for a request to run the code in the debugger. Seen so often, and always the wrong track. — πάντα ῥεῖ, Feb 22 '16 at 23:45
I can't remember when there was a linked-list question that I did not down and close vote for no aparrent debugging done:( — Martin James, Feb 22 '16 at 23:48
@MartinJames Yes these seem to be very prone to that specific issue actually. Managing pointers correctly is error prone doing runtime failures, and need debugging most of the time. — πάντα ῥεῖ, Feb 22 '16 at 23:50
@πάνταῥεῖ there is a small, but persistent, set of developers who consider that the use of a debugger is not necessary. I've had several arguments with them over the years, and never did find out which insane asylum they escaped from:( — Martin James, Feb 22 '16 at 23:53
@MartinJames _"set of developers who consider that the use of a debugger is not necessary"_ I'm mostly putting `cerr`/`logger` outputs first before starting the debugger actually. Especially to do rough narrowing of error sources (e.g. with many iterations before reaching the point of interest). The rest is to inspect the source code thoroughly again and use your brain. — πάντα ῥεῖ, Feb 22 '16 at 23:57

Colin Basnett · Accepted Answer · 2016-02-22T23:52:03.963

2

In your Insert function, you never allocate newNode.

Node* newNode;

You need to allocate it like so:

Node* newNode = new Node();

The program runs correctly after fixing this, and the output is:

http://ideone.com/16XL5W

EDIT: Regarding your comment below, the following lines do not allocate anything:

Node* newNode;
struct Node* newNode;

They are simply declaring variables, because you have identified them as pointers (*). Pointers simply point to an object on the heap. In order to actually create an object on the heap, you need to use new Node().

The reason your program crashed is because you were trying to access memory from a pointer that hadn't been initialized.

You can use malloc if you want, but you're programming in C++, so as a general rule, never use malloc unless you actually need to.

If you are using a compliant compiler, use shared_ptrs, then you never have to worry about deleteing things that you new.

edited Feb 22 '16 at 23:52

answered Feb 22 '16 at 23:44

Colin Basnett

4,052
2
30
49

Note that using shared pointers for linked lists [may have serious deficiencies](http://stackoverflow.com/questions/17804235/shared-pointers-delete-recursive-data-structures-recursively-and-the-stack-overf?s=1|0.7271) limiting these in allocation depth. – πάντα ῥεῖ Feb 23 '16 at 00:05
@πάνταῥεῖ A good point. In this case it wouldn't make much sense, but in more high-level applications, `shared_ptr`s are preferred in most cases. – Colin Basnett Feb 23 '16 at 00:07
I am reading C programming language right now. One of the example is Struct point makepoint(int x, int y) { struct point temp; ... return temp; } It does not do struct point temp = new point() – Telenoobies Feb 23 '16 at 00:23
In that case, it's calling a function that's returning a `point` object (not a pointer). You should really read up on the difference between heap and stack allocation. – Colin Basnett Feb 23 '16 at 00:25
Look at this guy's example. https://youtu.be/sYcOK51hl-A?t=12m4s In his reversal function, he just has struct Node *curr, *pre, *next. I am confused... – Telenoobies Feb 23 '16 at 00:31
Why is it that sometimes he can use Node* node, while other he uses struct Node* node? – Telenoobies Feb 23 '16 at 00:45
Functionally there's no difference in 99% of all cases. Generally, to reduce the amount of code, don't put `struct` in-front of the type unless you have to. This question (http://stackoverflow.com/questions/7729646/using-struct-keyword-in-variable-declaration-in-c) and the accepted answer has a good explanation as to why this is sometimes necessary. – Colin Basnett Feb 23 '16 at 00:49

score 0 · Answer 2 · answered Feb 22 '16 at 23:47

Aside from the Node* newNode allocation (uninitialized variable error), the following will throw a return value error:

Node* printList(Node* head)
{
    while (head)
    {
        cout << head->data;
        head = head->next;
    }
    cout << endl;
}

You have set Node* as the return type but this function has no return value. Because this function is printing out your list, it does not need to return anything. Change the return type to void.

Insert linked list

2 Answers2