Overloading assignment operator in Singly Linked List

Question

I'm learning about linked list. I created a template implementation, with a constructor, an inserter, a destructor, a copy constructor and an overloaded assignment operator. The problem is that my test programme doesn't output anything after overloading assignment operator.

For my assignment operator, I use a Clear() function to clear the list entirely before the copy. I put it in the destructor and checked that it works fine. I also checked my copy constructor and it worked fine as well.

File node.h: defines the node building block

#include <iostream>
using namespace std;

template <typename T>
struct Node{
    T _item;
    Node<T>* _next;

    Node() {
        _item = T();
        _next = NULL;
    }

    Node(T item){
        _item = item;
        _next = NULL;
    }

    // Print the value of a node
    friend std::ostream& operator <<(std::ostream& outs, const Node<T> &printMe){
        outs << "[" << printMe._item << "]";
        return outs;
    }
};

File list.h: defines the linked list template

#include "node.h"

template <class T>
class List {
public:

    // default constructor
    List();

    // Destructor
    ~List();

    // Copy constructor
    List(const List<T> &copyThis);

    // Overloading assignment operator
    List& operator =(const List& RHS);

    // Insert i to the head of the linked list
    Node<T>* InsertHead(T i);

    // Clear a linked list
    void Clear();

    // Overload the output operator to print the list
    template <class U>
    friend ostream& operator <<(ostream& outs, const List<U>& l);

private:
    Node<T>* head;
};

This header also provides the implementation of these member functions:

template <class T>
List<T>::List(){
    head = NULL;
}

template <class T>
List<T>::~List(){
    Clear();
}

template <class T>
List<T>::List(const List<T> &copyThis){
    if (copyThis.head == NULL)
        head = NULL;

    else {
        // Create walker for the original linked list
        Node<T>* walker = copyThis.head->_next;

        // Create new head node for new linked list
        head = new Node<T>(copyThis.head->_item);

        // Create new walker for new linked list
        Node<T>* new_walker = head;

        // Iterate walker and new walker and copy each item in the original list to new linked list
        while (walker!= NULL) {
            new_walker->_next = new Node<T>(walker->_item);
            walker = walker->_next;
            new_walker = new_walker->_next;
        }
    }
}

template <class T>
List<T>& List<T>::operator =(const List<T>& RHS){   // DOESN'T WORK
    if (this != &RHS) {
        this->Clear();
        *this = List<T>(RHS); 

    }
    return *this;
}

template <class T>
Node<T>* List<T>::InsertHead(T i){
    Node<T>* temp = new Node<T>(i);
    temp->_next = head;
    head = temp;
    return head;
}

// Clear a linked list
template <class T>
void List<T>::Clear(){
    Node<T>* current = head;
    Node<T>* next = new Node<T>;

    while (current != NULL) {
        next = current->_next;
        delete current;
        current = next;
    }

   head = NULL;
}

template <class U>
ostream& operator <<(ostream& outs, const List<U>& l){
    Node<U>* walker = l.head;

    while(walker != NULL){
        outs << *walker;
        outs << "->";
        walker = walker->_next;
    }

    outs << "|||";

    return outs;
}

File main.cpp: tests the classes

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

int main() {
    List<int> a;

    a.InsertHead(17);
    a.InsertHead(35);
    a.InsertHead(6);
    a.InsertHead(54);
    a.InsertHead(6);
    cout << a <<endl;;

    List<int> b;
    b.InsertHead(3);
    b.InsertHead(2);
    cout << b <<endl;;

    a = b;

    cout << a <<endl;        // PROBLEM: NOTHING IS DISPLAYED
    cout << b <<endl;
}

The problem I currently have is the overloading assignment operator function. Below is when I copy the whole execution from the copy constructor function and it runs.

template <class T>
List<T>& List<T>::operator =(const List<T>& RHS){
    if (this != &RHS) {
        this->Clear();
        if (copyThis.head == NULL)
            head = NULL;

        else {
            // Create walker for the original linked list
            Node<T>* walker = copyThis.head->_next;

            // Create new head node for new linked list
            head = new Node<T>(copyThis.head->_item);

            // Create new walker for new linked list
            Node<T>* new_walker = head;

            // Iterate walker and new walker and copy each item in the original list to new linked list
            while (walker!= NULL) {
                new_walker->_next = new Node<T>(walker->_item);
                walker = walker->_next;
                new_walker = new_walker->_next;
            }
    }
    return *this;
}

The output for this is:

2->3->|||

However, when I simplify the code like below, it doesn't output anything:

template <class T>
    List<T>& List<T>::operator =(const List<T>& RHS){
        if (this != &RHS) {
            this->Clear();
            *this = List<T>(RHS); 

        }
        return *this;
    }

Can anyone tell me why it doesn't work and how to simplify it efficiently? I really appreciate it.

When you call `*this = List(RHS);` what code do you expect to run? — Tim Randall, Oct 29 '18 at 19:26
@Tim Randall: After I clear the current object, I want to copy the whole thing from RHS to current object. — Brandon Williams, Oct 29 '18 at 19:29
Or you could just use `std::list` or `std::forward_list` or (usually better) `std::vector`. — Jesper Juhl, Oct 29 '18 at 19:29
@Jesper Juhl: I'm not allowed to use that in my code. I also just want to understand why I can't use *this = List(RHS); I thought it would copy the whole thing from RHS, because I have the copy constructor above — Brandon Williams, Oct 29 '18 at 19:32
Please post real code. There's no `copyThis` declared in the (long) assignment operator. — n. m. could be an AI, Oct 29 '18 at 19:35
*I want to copy the whole thing from RHS to current object.* That's exactly what the assignment operator does. So you are saying "my assignment operator does what my assignment operator does". This looks a bit circular to me. — n. m. could be an AI, Oct 29 '18 at 19:37
@n.m. You can put that in the execution in the file list.h above — Brandon Williams, Oct 29 '18 at 19:42
"... it doesn't output anything..." Are you saying that the program just terminates normally with out any print? — Support Ukraine, Oct 29 '18 at 19:43
@n.m. So I assume that once I clear the whole object (which makes the object default), I can use copy constructor to paste what is in RHS to current object — Brandon Williams, Oct 29 '18 at 19:45
@BrandonWilliams Have you tried to add some unconditional prints before and after printing the list: `cout << "PRINT LIST:"; ; cout << a; cout << "PRINTED";` Have you tried some prints inside the various function to see what the code is doing? — Support Ukraine, Oct 29 '18 at 19:48
*I can use copy constructor*. You *can*, but you are *not doing it*. You are using the assignment operator. The very assignment operator function you are defining. In order to use the copy constructor in the assignment operator, you must specifically learn how to do that (it's not exactly obvious). — n. m. could be an AI, Oct 29 '18 at 20:05
See [What is the copy-and-swap idiom?](https://stackoverflow.com/questions/3279543/what-is-the-copy-and-swap-idiom) for a good treatment of the idea. — n. m. could be an AI, Oct 29 '18 at 20:26

Christophe · Answer 1 · 2018-10-29T20:27:54.220

The problem

The assignment operator stops everything because of a stack overflow.

In fact, your implementation of the assignment operator uses itself the assignment operator, so that it calls itself recursively until the stack is exhausted:

    *this       =    List<T>(RHS);  // OUCH !!
      |         |          |
      V         V          V
    <ListT> operator=   List<T>   ==> cals operator= again !!

The solution

Rewrite the operator so that it doesn't call itself.
Possibly clone every node, so to avoid that 2 lists share the same node and tha the first one that frees its node causes dangling pointers and UB for the other.
Not related, but please avoid using namespaces in headers. This is an extremely bad habit for later.

Additional tip: This article recommends some good and elegant practices for operator overloading. For the assignment operator, it suggests to use the copy constructor (as you attempt to do) but instead of assigning (your problem), swapping (to be verified, but swapping the heads in your case would certainly do the trick).

@BrandonWilliams you're welcome. I made an edit with an interesting suggestion. — Christophe, Oct 29 '18 at 20:29

Overloading assignment operator in Singly Linked List

1 Answers1

The problem

The solution