Insertion in linked list creating loop

Question

So, I am trying to implement a simple linked list in C++, but I am having a problem with the push method of my class. Basically, when I add the first node to a list, everything goes fine; but, when I add a second node, it ends up pointing to itself (ie, secondNode.next == &secondNode).

class linkedList
{
    public:
    node head;
    linkedList()
    {
        head.next = NULL;
    }
    void push(node new)
    {
        if(head.next == NULL)
        {
            head.next = &new;
            new.next = NULL;
        }
        else
        {
            new.next = head.next;
            head.next = &new;
        }
    }
};

I couldn't figure out what is wrong... Any help would be greatly appreciated.

That can't be C++, it must be C because new is a reserved keyword. — K-ballo
– K-ballo, Commented May 26, 2012 at 23:34
@K-ballo: But if it was c, the class linkedList wouldn't be allowed -- unless you had something like #define class struct it wouldn't even compile. — Jerry Coffin
– Jerry Coffin, Commented May 26, 2012 at 23:37
At any rate, don't use the word new as a variable name in C++. — Asherah
– Asherah, Commented May 26, 2012 at 23:39
Sorry, it was not "new" in the original code. I translated the variable names from portuguese to english, originally it was "novo" — bluewhale
– bluewhale, Commented May 26, 2012 at 23:47

lezebulon · Accepted Answer · 2012-05-26 23:45:09Z

2

void push(node new)

you have to not make a copy of the object, like so:

void push(node& new)

otherwise you are taking the adress of an object that is deleted at the end of the function

edited May 26, 2012 at 23:45

answered May 26, 2012 at 23:35

lezebulon

8,11411 gold badges49 silver badges86 bronze badges

Sign up to request clarification or add additional context in comments.

Comments

Jerry Coffin · Accepted Answer · 2012-05-27 00:15:16Z

At least in my opinion, you have a few things that are wrong to some degree or other.

First, head shouldn't really be a node -- it should be a node *. At least from the looks of things, all you're ever using it for is its next pointer, so you might as well just make it a pointer and be done with it.

Second, to insert new items at the beginning of the list, you don't really need to check for whether the head of the list is a null pointer or not.

Third, although @lezebulon's suggestion to use a reference to a node will work, I don't think it's really the best way to go in this case. Rather than having the user pass a pointer or reference to a node, they should really just pass in a data item, and your linked-list class should allocate a node to hold that item in the list.

template <class T>
class linked_list { 

    class node { 
        T item;
        node *next;
    public:
        node(T const &data, node *next_node) : item(data), next(next_node) {}
    };

    node *head;
public:

    linked_list() : head(NULL) {}

    void push(T const &data) {
        head = new node(data, head);
    }
};

Collectives™ on Stack Overflow

Insertion in linked list creating loop

2 Answers 2

Comments

Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related