Singly Linked List using shared_ptr

Question

I was trying to implement singly linked list using share_ptr. Here is the implementation... Below is the node class...

template<typename T>
class Node
{
public:
    T value;
    shared_ptr<Node<T>> next;
    Node() : value(0), next(nullptr){};
    Node(T value) : value(value), next(nullptr){};
    ~Node() { cout << "In Destructor: " << value << endl; };
};

Below is the linked list class...

template<typename T>
class LinkedList
{
private:
    size_t m_size;
    shared_ptr<Node<T>> head;
    shared_ptr<Node<T>> tail;
public:
    LinkedList() : m_size(0), head(nullptr) {};
    void push_front(T value)
    {
        shared_ptr<Node<T>> temp = head;
        head = make_shared<Node<T>>(Node<T>(value));

        head->next = temp;
        m_size++;

        if (m_size == 1)
            tail = head;
    }
    void pop_front()
    {
        if (m_size != 0)
        {
            // Here I am having doubt------------------------!!!
            //shared_ptr<Node<T>> temp = head;
            head = head->next;
            m_size--;
            if (m_size == 0)
                tail = nullptr;
        }
    }
    bool empty()
    {
        return (m_size == 0) ? true : false;
    }
    T front()
    {
        if (m_size != 0)
            return head->value;
    }
};

My question is, am I using the shared_ptr properly for allocating a node? If not, how should I use the shared_ptr to allocate and how should I delete the node in the pop_front method?

Answer 1

I believe this belongs on code review.

Most importantly: Why are you using shared_ptr? shared_ptr means the ownership of an object is unclear. This is not the case for linked lists: Every node owns the next. You can express that using unique_ptr which is easier and more efficient.

pop_front seems to be functioning correctly. You may consider throwing an exception or an assertion instead of doing nothing when using pop_front on an empty list.

front is more problematic. If the list is empty you most likely get a garbage object.

What is the significance of tail? It does not seem to be used for anything and since you cannot go backwards there is no real point to getting the tail.

make_shared<Node<T>>(Node<T>(value)) should be make_shared<Node<T>>(value) instead. make_shared<Node<T>>(value) creates a Node using value as the parameter for the constructor. make_shared<Node<T>>(Node<T>(value)) creates a Node with value as the parameter and then creates a new Node with the temporary Node as parameter and then destroys the first Node.

You are missing the copy and move constructor and assignment and move assignment operators.

After you are satisfied with your list implementation consider using std::forward_list instead.