C++ Linked list only print the first 3 nodes

Question

When you run the code, it won't print anything unless when you run it with 3 appends. Why is that? Inside the code, I added cout statement to check if it ran and when I appended 4 things to the linked list, it only ran once in the append function. But when I ran it with only 3 things appended to the list, it displayed the cout statement 3x.

main.cpp:

#include <iostream>
#include "node.h"
using namespace std;

int main()
{
    LL list;
    list.append("jack","2");
    list.append("jack","1");
    list.append("jack","3");
    list.append("jack","4");
    //list.insertatBegin("notjack","0");
    list.print();
}

node.cpp:

#include <iostream>
using namespace std;
#include "node.h"

LL::LL()
{
    head = nullptr;
}

void LL::append(string pName,string phone)
{
    Node *nodePtr;
    if (head == nullptr)
    {
        head = new Node;
        head->name = pName;
        head->phoneNumber = phone;
        head->next = nullptr;
    }
    else
    {
        nodePtr = head;
        while(nodePtr->next !=nullptr)
        {
            nodePtr = nodePtr->next;
        }
        nodePtr->next = new Node;
        nodePtr->next->name = pName;
        nodePtr->next->phoneNumber = phone;
        nodePtr->next->next = nullptr;
    }

}

void LL::print()
{
    //cout << "ran" <<endl;
    Node *nodePtr;
    nodePtr = head;
    while (nodePtr == nullptr)
    {
        cout << nodePtr ->name << " " << nodePtr->phoneNumber <<endl;
        nodePtr = nodePtr->next;
    }
}

node.h:

#ifndef NODE_H
#define NODE_H
#include <iostream>
using namespace std;

class Node
{
public:
    string name; //data
    string phoneNumber;
    Node* next;  //pointer to next

};

class LL
{
private:
    Node* head; // list header
public:
    LL();
    void append(string pName,string phone);
    void insertatBegin(string pName,string phone);
    void print();
};


#endif

In your `append` function you have the pointer `newNode`. But ***where does it point?*** Uninitialized variables really *are* uninitialized, and will have an *indeterminate* value (which might seem like random or garbage). Using such an indeterminate value leads to *undefined behavior*. — Some programmer dude, Apr 28 '20 at 22:16
I really don't like abbreviations. A common definition for `LL` is `long long` as in integer. Any problems with spelling it out? You have at least 32 characters in length to use. — Thomas Matthews, Apr 28 '20 at 22:36
Unrelated to your problem, but please read [Why is “using namespace std;” considered bad practice?](https://stackoverflow.com/questions/1452721/why-is-using-namespace-std-considered-bad-practice) Doing it in a header file is double the badness. — Some programmer dude, Apr 28 '20 at 22:37
You are going to have name collisions. You have opened up the *entire* `std` namespace and that also included `std::list`. You may experience name conflicts between your `list` and the `list` in the standard namespace. — Thomas Matthews, Apr 28 '20 at 22:37
If you don't like typing `std::` all the time, you can specify individual items from the `std` namespace, e.g. `using std::string;` — Thomas Matthews, Apr 28 '20 at 22:38

Remy Lebeau · Answer 1 · 2020-04-28T23:08:59.190

There are 2 problems with your code:

append() has undefined behavior, because newNode is uninitialized. Its value is indeterminate, causing it to point at random memory. You are not pointing it to a valid new'ed instance of Node before trying to populate it.
print() is not looping through the list at all.

Try this:

void LL::append(string pName,string phone)
{
    Node *newNode = new Node; // <-- notice 'new'!

    // these assignments really should be handled by a constructor...
    newNode->name = pName;
    newNode->phoneNumber = phone;
    newNode->next = nullptr;

    if (head == nullptr)
    // better: if (!head)
    {
        cout << "it ran" <<endl;
        head = newNode;
    }
    else
    {
        cout << "it ran2" <<endl;
        Node *nodePtr = head;
        while (nodePtr->next != nullptr)
        // better: while (nodePtr->next)
        {
            nodePtr = nodePtr->next;
        }
        nodePtr->next = newNode;
    }
}

void LL::print()
{
    //cout << "ran" <<endl;
    Node *nodePtr = head;
    while (nodePtr != nullptr) // <-- '!=', not '=='
    // better: while (nodePtr)
    {
        cout << nodePtr ->name << " " << nodePtr->phoneNumber << endl;
        nodePtr = nodePtr->next;
    }
}

That said, append() can be simplified a bit more:

class Node
{
public:
    string name; //data
    string phoneNumber;
    Node* next = nullptr;  //pointer to next

    Node(string pName, string phone) : name(pName), phoneNumber(phone) {}
};

void LL::append(string pName,string phone)
{
    Node *newNode = new Node(pName, phone);
    Node **nodePtr = &head;
    while (*nodePtr)
    {
        nodePtr = &((*nodePtr)->next);
    }
    *nodePtr = newNode;

    // Alternatively:
    /*
    Node **nodePtr = &head;
    while (*nodePtr)
    {
        nodePtr = &((*nodePtr)->next);
    }
    *nodePtr = new Node(pName, phone);
    */
}

Like I said in my answer, if you do not `new` the Node, then `newNode` is not pointing at valid memory. You have to assign a pointer to actually point at allocated memory before you can use that pointer to fill it that memory. — Remy Lebeau, Apr 28 '20 at 23:01

score 1 · Answer 2 · answered Apr 28 '20 at 22:29

1

For starters your newNode pointer does not point to anything and you are assigning to uninitialized variables name, phonenumber and next.

Node *nodePtr;
newNode->name = pName;
newNode->phoneNumber = phone;
newNode->next = nullptr;

answered Apr 28 '20 at 22:29

Bugman

191
6

C++ Linked list only print the first 3 nodes

2 Answers2