SinglyLinkedList implementation with rule of 3

Question

I'm new to C++ and have been trying to implement a Singly Linked List, that provides an implementation the destructor, copy constructor and assignment operator. I'm running into compilation issues when trying to implement the copy constructor and the assignment operator.

Here's node.hpp

#ifndef LINKED_LIST_NODE_HPP
#define LINKED_LIST_NODE_HPP

template <typename T>
class Node{
public:
    T data;
    Node* next;
    Node();
    Node(T);
    Node(const Node&);
    ~Node();

};

template <typename T>
Node<T>::Node(){}

template <typename T>
Node<T>:: Node(const T data): data(data), next(nullptr){}

template <typename T>
Node<T>::Node(const Node<T>& source) : data(source.data),
                         next(new Node)
{
  (*next) = *(source.next) ;
}

template <typename T>
Node<T>::~Node(){}


#endif //LINKED_LIST_NODE_HPP

This is singly_linked_list.hpp

#ifndef LINKED_LIST_SINGLYLINKEDLIST_HPP
#define LINKED_LIST_SINGLYLINKEDLIST_HPP

#include <iostream>
#include "node.hpp"

template <typename T>
class SinglyLinkedList {

private:
    Node<T>* head;
    std::size_t count;

public:
    SinglyLinkedList();
    SinglyLinkedList(const SinglyLinkedList& source);
    SinglyLinkedList& operator=(const SinglyLinkedList& source);
    ~SinglyLinkedList();
    void insert(T);
    void remove(T);
    bool isEmpty();
    int length();
    void print();

};

template <typename T>
SinglyLinkedList<T>::SinglyLinkedList() : head(nullptr), count(0){}

template <typename T>
template <typename T>
SinglyLinkedList<T>::SinglyLinkedList(const SinglyLinkedList& source){

    Node<T>* curr = source.head;

    while(curr != nullptr){
        Node<T>* p = new Node<T>;
        p->data = curr->data;
        curr = curr->next;

    }

}

//template <typename T>
//SinglyLinkedList<T>::SinglyLinkedList& operator=(const SinglyLinkedList<T>& source){
//    //not sure how to implment this.
//}

template <typename T>
SinglyLinkedList<T>::~SinglyLinkedList() {
    if(!isEmpty()){
        Node<T>* temp = head;
        Node<T>* prev = nullptr;
        while(temp->next != nullptr){
            prev = temp;
            temp = temp->next;
            delete prev;
        }
        delete temp;
    }
}

template <typename T>
bool SinglyLinkedList<T>::isEmpty() {
    return head == nullptr;
}

template <typename T>
void SinglyLinkedList<T>::insert(T item) {
    Node<T>* p = new Node<T>(item);
    p->next = head;
    head = p;
    count += 1;

}

template <typename T>
void SinglyLinkedList<T>::remove(T item) {
    bool present = false;
    if (head->data == item){
        Node<T>* temp = head;
        head = head->next;
        delete(temp);
        count -= 1;
        return;
    }
    Node<T>* temp = head;
    while (temp->next != nullptr){
        if (temp->next->data == item){
            Node<T>* removable = temp->next;
            temp->next = temp->next->next;
            delete(removable);
            present = true;
            count -= 1;
            break;
        } else{
            temp = temp->next;
        }
    }
    if(!present){
        throw std::invalid_argument("item not present in list");
    }
}

template <typename T>
int SinglyLinkedList<T>::length() {
    return count;
}

template <typename T>
void SinglyLinkedList<T>::print() {
    if(isEmpty()){
        throw std::invalid_argument("Can't print an empty list!");
    }
    Node<T>* temp = head;
    while(temp != nullptr){
        if(temp->next != nullptr){
            std::cout<<temp->data;
            std::cout<<"->";
        }else{
            std::cout<<temp->data;
        }
        temp = temp->next;
    }
    std::cout<<std::endl;

}

#endif //LINKED_LIST_SINGLYLINKEDLIST_HPP

I've commented out the copy constructor code to make this compile. What is the correct way of doing this? I'm just learning C++.

Answer 1

One issue that introduces complexity is that it is not well defined what the copy constructor of a node should do? Should the next field of the copy point to the next of the original, or it should create a copy of the next and point to that? The former is inadequate and error-prone, the latter would recursively create a copy of the whole list, one node at a time. This will work for lists of small size but will cause stack overflow for lists with many elements due to the depth of the recursive calls.

So to keep things simple, I wouldn't bother with copy constructor of a node.

template <typename T>
class Node {
public:
    T data;
    Node* next = nullptr;

    Node() = default;
    Node(const Node&) = delete; // since copying is not well defined, make it impossible to copy a node.
};

Copying a list is a well defined operation, so implementing the copy constructor makes sense. A mistake with your current implementation is that you allocate a new node, only to leak it later (nothing keeps track of the newly allocated node p). What you need looks more like this:

template <typename T>
SinglyLinkedList<T>::SinglyLinkedList(const SinglyLinkedList<T>& source)
    : head(nullptr)
    , count(0)
{
    // deal with the trivial case of empty list
    if (source.head == nullptr)
        return;

    // deal with the case where count >= 1
    head = new Node<T>;
    head->data = source.head->data;
    head->next = nullptr;
    count = 1;

    Node<T>* lastCopied = source.head;  // last node to be copied
    Node<T>* lastAdded = head;         // last node to be added to the current list
    while (lastCopied->next != nullptr)
    {
        // create new node
        Node<T>* p = new Node<T>;
        p->data = lastCopied->next->data;
        p->next = nullptr;

        // link the newly created node to the last of the current list
        lastAdded->next = p;
        lastAdded = p;

        // advance lastCopied
        lastCopied = lastCopied->next;
        count++;
    }
}

Now regarding the assignment operator, luckily you can use the 'copy and swap' idiom that greatly simplifies things.

template <typename T>
SinglyLinkedList<T>& SinglyLinkedList<T>::operator =(SinglyLinkedList<T> source)  // note that you pass by value.
{
    std::swap(head, source.head);
    std::swap(count, source.count);
    return *this;
}

My answer would become too long if I tried to explain the copy and swap technique. It is a clever trick to write exception safe code and avoid duplication (implements assignment by using the copy ctor) at the same time. It is worth reading about it here.

Btw, the declaration of your class should look like this

template <typename T>
class SinglyLinkedList 
{

private:
    Node<T>* head = nullptr;
    std::size_t count = 0;

public:
    SinglyLinkedList(const SinglyLinkedList& source);
    SinglyLinkedList& operator=(SinglyLinkedList source);
    // other members here...
};

PS. My code assumes you are using c++11 or a later standard.

Answer 2

I don't like the direction this is headed. I'm going to explain how to do this approach right because it is an excellent lesson on recursion, but because it's recursion it can run the program out of Automatic storage (march off the end of the stack, most likely) with a sufficiently large list. Not cool.

The logic:

Copying a node copies the next node if there is one. This looks something like

template <typename T>
Node<T>::Node(const Node<T>& source) : data(source.data)
{
    if (source.next) // if there is a next, clone it
    {
        next = new Node<T>(*source.next);
    }
    else
    {
        next = nullptr;
    }
}

This reduces the linked list copy constructor to

template <typename T>
SinglyLinkedList<T>::SinglyLinkedList(const SinglyLinkedList& source){

    head = new Node<T>(*source.head); //clone the head. Cloning the head will clone everything after
    count = source.count;
}

A helper function may, uh... help here to make the Node copy constructor a bit more idiomatic

template <typename T>
Node<T> * initnext(const Node<T> & source)
{
    if (source.next)
    {
        return new Node<T>(*source.next);
    }
    else
    {
        return nullptr;
    }
}

template <typename T>
Node<T>::Node(const Node<T>& source) : data(source.data),
                         next(initnext(source))
{
}

but I don't think you gain much.

So... I don't like the above. What would I do instead? Something a lot like opetroch's solution above, but different enough that I'll write this up.

The node stays brutally stupid. As far as I'm concerned all a Node should ever know is how to store the payload and find other Nodes. This means the linked list should do all of the heavy lifting.

Concept 1: head is nothing but a next pointer. Once you abstract away its differences, unimportant here, you can use it exactly the same way you would next.

Concept 2: If you only know where next points, you have to do a bunch of extra book-keeping to track the previous node to update it's next. But if you take advantage of the previous's next pointing to the current node, you can throw out even more code. By tracking the previous node's next you have all of the information you need.

Concept 3: If you keep a pointer to the previous node's next, you can update that previous node's next any time you want by dereferencing it.

template <typename T>
SinglyLinkedList<T>::SinglyLinkedList(const SinglyLinkedList& obj)
{
    Node<T>* tocopy = obj.head;  
    Node<T>** nextpp = &head; // head is a next. We are now pointing to a pointer to next
    while (tocopy) // keep looping until there is no next node to copy
    {
        *nextpp = new Node<T>(tocopy->data); // copy source and update destination's next
        nextpp = &(*nextpp)->next; // advance to point at the next of the node we just added
        tocopy= tocopy->next; // get next node to copy
    }
    count = obj.count;
}

Because this iterates rather than recurses it doesn't eat up Automatic storage (probably the stack) and can keep going until the cows come home.

This logic can also be applied to remove

template <typename T>
void SinglyLinkedList<T>::remove(T item) {
    Node<T>** temp = &head; //head is nothing but a next pointer.
                            // by pointing to where the next is, we don't
                            // need to track a previous or have special handling
                            // for the head node
    while (*temp){ // because we now have a pointer to a pointer, we need an
                   // extra dereference
        if ((*temp)->data == item){
            Node<T>* removable = *temp;
            *temp = (*temp)->next;
            delete(removable);
            count -= 1;
            return; // no need for any special magic. Just get out.
        } else{
            temp = &(*temp)->next; // update the pointer to the next
        }
    }
    // if we got here the node was not found.
    throw std::invalid_argument("item not present in list");
}

And following through on head is just a next, we can also gut the destructor:

template <typename T>
SinglyLinkedList<T>::~SinglyLinkedList() {
    while(head){ // if head null, list empty
        Node<T>* temp = head; // cache so we can delete
        head = head->next; // move head
        delete temp; //delete removed node
    }
}