Can someone help me with this linked list? C++

Question

I have an object oriented exam tomorrow and I'm still stuck with linked lists. For some reason I cant quite understand how they work or how to implement them properly. I've watched a lot of videos and tutorials but it still is quite complex. I always get an error which prints nothing to the console, just a blank black page. Can someone tell em what am I doing wrong? Thank you.

#include <iostream>
#include <string>

using namespace std;

struct node
{
    string name;
    int number;
    node* next;
};
struct node* head = 0;

class Employees
{
private:
    node* head, * tail;
public:
    Employees()
    {
        head = NULL;
        tail = NULL;
    }
    void addToList(string Name, int Num)
    {
        node* n = new node;
        n->name = Name;
        n->number = Num;
        n->next = NULL;
        if (head == NULL)
        {
            head = n;
            tail = n;
        }
        else
        {
            tail->next = n;
            tail = tail->next;
        }
    }
    void PrintAll()
    {
        while (head != NULL)
        {
            node* current;
            while (current != NULL)
            {
                cout << current->name << "\t";
                cout << current->number;
            }
        }
    }
};

int main()
{
    Employees a;
    a.addToList("Robert", 54);
    a.addToList("Manny", 77);

    a.PrintAll();

}

Answer 1

For starters this declaration in the global namespace

struct node
{
    string name;
    int number;
    node* next;
};
struct node* head = 0;
^^^^^^^^^^^^^^^^^^^^^

is redundant and is used nowhere. Remove it.

It is better to make the structure node an inner member of the class Employees. For example

class Employees
{
private:
    struct node
    {
        string name;
        int number;
        node* next;
    } *head = nullptr, *tail = nullptr;
    //...   

The constructor does nothing special. So it can be defined as a default constructor.

 Employees() = default;

Th function addToList should accept the first argument by constant reference

void addToList( const string &Name, int Num )
                ^^^^^^^^^^^^^^^^^^

The function PrintAll has an infinite loop when the pointer head is not a null pointer

void PrintAll()
{
    while (head != NULL)
    {
        //...
    }
}

and moreover it invokes undefined behavior because the used pointer current was not initialized

node* current;
while (current != NULL)

The function should be declared with the qualifier const because it does not change the list itself.

Also you need a destructor to free the dynamically allocated memory for nodes. Also you could suppress the copy construction and assignment.

Here is a demonstrative program that shows how the class can be implemented.

#include <iostream>
#include <string>

using namespace std;

class Employees
{
private:
    struct node
    {
        string name;
        int number;
        node* next;
    } *head = nullptr, *tail = nullptr;

public:
    Employees() = default;

    ~Employees()
    {
        while ( head != nullptr )
        {
            node *tmp = head;
            head = head->next;
            delete tmp;
        }
        tail = nullptr;
    }

    Employees( const Employees & ) = delete;
    Employees & operator =( const Employees & ) = delete;

    void addToList( const string &Name, int Num )
    {
        node *n = new node { Name, Num, nullptr };

        if ( head == nullptr )
        {
            head = n;
            tail = n;
        }
        else
        {
            tail->next = n;
            tail = tail->next;
        }
    }

    void PrintAll() const
    {
        for ( node *current = head; current != nullptr; current = current->next )
        {
            cout << current->name << ' ';
            cout << current->number << '\t';
        }
    }
};

int main()
{
    Employees a;
    a.addToList( "Robert", 54 );
    a.addToList( "Manny", 77 );

    a.PrintAll();

    cout << endl;
}

The program output is

Robert 54   Manny 77

Answer 2

Three issues:

while (head != NULL)
{
    /*...*/
}

You do not modify head inside the loop (why would you), hence this loop would run either never or forever.

Second:

 node* current;
 while (current != NULL)

current in uninitialized. Comparing it to NULL invokes undefined behavior. Always initialize your variables!

Last:

while (current != NULL)
{
    cout << current->name << "\t";
    cout << current->number;
}

Similar to the first issue, the loop condition is either true or false, but it never changes, ie the loop either runs never or infinite.

As this appears to be an exercise, I'll leave it to you to fix the code.

Answer 3

The primary problem with your code is that your PrintAll method is written incorrectly and you never update your current pointer. Perhaps you've not been taught for loops yet, but this is an ideal case for them:

for(node* current = head; current != NULL; current = current->next) {
  cout << current->name << "\t" << current->number;
}

The first part of the for loop initialises the variable (which you neglected to do in your code), the next is the end condition (as you'd find in a while loop), and the third part updates the variable each cycle through the loop thus traversing your loop from start to end.

You can do all this with a while loop but it expresses your intent less clearly, so a for loop should be preferred. I am also not sure why you have a while loop checking whether head is null? Since it does not change during the loop, there is no need to repeatedly check it, and - in any case - there is no advantage to checking it separately as opposed to simply checking current once it has been initialised to head.

As a minor note, if you're using a modern C++ version, nullptr should be preferred over NULL.

Answer 4

You have issue with

1. PrintAll where Head is unaltered and makes it infinite loop.
2. current is uninitialised and accessing it as a pointer is UB

Below snippet shoud fix your problem,

void PrintAll()
{
    node* temp = head;
    while (temp != nullptr)
    {
       std::cout << temp ->name << "\t";
       std::cout << temp ->number;
       temp = temp->next            
    }
}