Printing the elements of a forward linked list

Question

I'm a sophomore in computer science who's working on a linked list class for his DSA class. I believe this code can be improved, but I'm not sure how because everything got really messy really fast. Thanks.

List.hpp:

// This is a blueprint for all list-like data structures.
// It won't specify how the operations are carried out.
#ifndef LIST_HPP
#define LIST_HPP

#include <cassert>
#include <initializer_list>

template <typename T>
class List
{
public:
    List()
    {}

    virtual ~List()
    {}

    List(const List&)
    {}

    virtual void insert(int pos, const T& item) = 0;
    virtual void append(const T& item) = 0;

    virtual T remove(const int pos) = 0;

    virtual int size() const = 0;

    virtual T& operator[](const int pos) = 0;
    // The const overload is called only when used on const object.
    virtual const T& operator[](const int pos) const = 0;

};
#endif // LIST_HPP

LinkedList.hpp:

#ifndef LINKEDLIST_HPP
#define LINKEDLIST_HPP

#include "List.hpp"
#include "Node.hpp"

#include <iostream>

template <typename T>
class LinkedList: public List<T>
{
private:
    Node<T>* m_head{nullptr};
    Node<T>* m_tail{nullptr};

    int m_size{};

public:
    LinkedList() = default;

    LinkedList(std::initializer_list<T> i_list)
    {
        for (const auto& item : i_list)
        {
            append(item);
        }
    }

    ~LinkedList()
    {
        // Start from the beginning.
        Node<T>* temp = m_head;

        // Traverse the list while deleting previous elements.
        while (temp != nullptr)
        {
            temp = temp->next; // Move forward.
            delete m_head;     // Delete the previous element.
            m_head = temp;     // m_head moved one forward.
        }
    }

    auto head() const
    {
        return m_head;
    }

    auto tail() const
    {
        return m_tail;
    }

    bool empty() const
    {
        return m_size == 0;
    }

    int size() const
    {
        return m_size;
    }

    void prepend(const T& item)
    {
        // Create a node holding our item and pointing to the head.
        auto new_item = new Node<T>{item, m_head};

        // If the head is the last element
        // (meaning it is the only element),
        // it'll be the tail.
        if (m_head == nullptr)
        {
            m_tail = m_head;
        }

        m_head = new_item;
        m_size++;
    }

    void append(const T& item)
    {
        // Create a node with no pointer.
        auto new_item = new Node<T>{item};

        if (m_tail == nullptr)
        {
            // If the list is empty, set the new item as the head as well.
            m_head = new_item;
            m_tail = new_item;
        }
        else
        {
            // Otherwise, update the current tail's next pointer to the new item and move the tail.
            m_tail->next = new_item;
            m_tail = new_item;
        }

        m_size++;
    }

    void insert(const int pos, const T& item)
    {

        // If the position is the beginning of the list, prepend the new node.
        if (pos == 0)
        {
            prepend(item);
        }
        // If the position is beyond the end of the list, append the new node.
        else if (pos >= m_size)
        {
            append(item);
        }
        else
        {
            // Create a new node.
            auto new_item = new Node<T>{item};

            // Starting from the head, go to the one previous position.
            auto temp = m_head;

            int i{};
            for (; i < pos - 1; ++i)
            {
                temp = temp->next;
            }

            new_item = temp->next;
            temp->next->next = new_item->next;

            m_size++;
        }
    }

    void remove_front()
    {
        Node<T>* temp = m_head;

        if (temp != nullptr)
        {
            return;
        }

        m_head = m_head->next; // Move m_head one element forward.
        delete temp;           // Get rid of the previous element.

        --m_size;
    }

    void remove_back()
    {
        Node<T>* temp = m_head;

        if (temp == nullptr)
        {
            return;
        }

        // If the list's size is 1.
        if (temp == m_tail)
        {
            delete temp;
            m_head = m_tail = nullptr;

            --m_size;

            return;
        }

        // Traverse to one past the end.
        while (temp->next != m_tail)
        {
            temp = temp->next;
        }

        m_tail = temp;
        temp = temp->next;
        delete temp;

        m_tail->next = nullptr;

        --m_size;
    }

    T remove(const int pos)
    {
        Node<T>* temp = m_head;

        int i{};
        for (; i < pos - 1; ++i)
        {
            temp = temp->next;
        }

        auto to_removed = temp->next;
        temp->next = temp->next->next;

        T removed_data = to_removed->data; // Retrieve the data before deleting the node.

        delete to_removed;
        return removed_data;
    }

    T& operator[](const int pos)
    {
        Node<T>* temp = m_head;

        int i{};
        for (; i != pos; ++i)
        {
            temp = temp->next;
        }

        return temp->data;
    }

    const T& operator[](const int pos) const
    {
        Node<T>* temp = m_head;

        int i{};
        for (; i != pos; ++i)
        {
            temp = temp->next;
        }

        return temp->data;
    }
};

template <typename T>
std::ostream& operator<<(std::ostream& os, const LinkedList<T>& list)
{
    for (auto begin = list.head(); begin != nullptr; begin = begin->next)
    {
        os << begin->data << ' ';
    }
    return os;
}

#endif // LINKEDLIST_HPP

main.cpp:

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

int main()
{
    LinkedList<int> l{1,2,4};

    for (int i{}; i < 10; ++i)
    {
        l.append(i * i);
    }
    std::cout << l << "\n";
}
```

Answer 1

The code is not too bad, so I think you're well on your way to mastering C++. Here are a few things to consider.

Be wary of signed versus unsigned

What happens if you specify a negative number for the position? For example, l.remove(-9);? I think you'll find that nothing good will happen there. The two options you have are to check for and reject negative numbers or to declare it as unsigned to avoid the issue entirely.

Use only necessary #includes

The #include <cassert> line in list.hpp is not necessary and can be safely removed.

Consider returning std::size_t for size()

The standard containers tend to use std::size_t for the return type for size() and that is probably a good choice for your code, too.

Consider range checking

Both of your operator[] return a reference and do no range checking, so that if one asks for l[99] the code will invoke undefined behavior. This isn't necessarily a problem (the standard containers to the same thing) but you might consider documenting this fact and/or implementing an at() function that does bounds checking, as the standard containers do.

Minimize the scope of variables

In a few places, you have something like this:

        int i{};
        for (; i != pos; ++i)
        {
            temp = temp->next;
        }

That's not wrong, but I think I would write it like this instead:

    for (int i{0}; i != pos; ++i) {
        temp = temp->next;
    }

In that version, the scope of i is solely within the for loop. Another idiomatic option is this:

    for ( ; pos; --pos) {
        temp = temp->next;
    }

Make sure your comments don't mislead

There is a comment in remove_back() that seems misleading to me.

        // Traverse to one past the end.
        while (temp->next != m_tail)
        {
            temp = temp->next;
        }

In fact, it's traversing to one before the end.

Consider slightly streamlining the code

In a few places, there are extra assignments that aren't needed. For example, in remove_back we have this:

        m_tail = temp;
        temp = temp->next;
        delete temp;

        m_tail->next = nullptr;

A slightly simpler version is this:

        m_tail = temp;
        delete m_tail->next;
        m_tail->next = nullptr;

Reconsider the use of const arguments

The arguments for operator[] and remove() take const int arguments for the pos argument, but there's really no need for restricting the caller in this way since we're passing by value anyway.

Use override or final on overridden functions

Explicitly using either final or override can help detect errors, and so you should use these where appropriate. See C.128 for details.

Fix the bug

The prepend() function contains this code:

        // If the head is the last element
        // (meaning it is the only element),
        // it'll be the tail.
        if (m_head == nullptr)
        {
            m_tail = m_head;
        }

I think that if you think about this, you'll see that it's a bug.