Generic Singly Linked List in C++ I am currently working on implementing some data structures as a way to learn C++. I'm coming mostly from a Java background so I was hoping to get some feedback regarding this implementation of a singly linked list in C++ using templates. This singly linked list will only support insertions at the head and tail positions and deletions from the head position. The plan is to use this as a base to build further data structures. I would like to create a rather complete library of data structures, so I was hoping to make them as extensible and reusable as possible. 

Below follows the class definition. 

#ifndef LINKEDLISTS_SINGLYLINKEDLIST_H_
#define LINKEDLISTS_SINGLYLINKEDLIST_H_

#include &lt;cstddef&gt;
#include &lt;iostream&gt;

template &lt;typename E&gt;
class SinglyLinkedList {

private:

    /** BEGIN NESTED NODE CLASS **/
    class Node {

    private:
        E data; //Should this be a reference?
        Node* next;

    public:
        Node(Node* n, const E&amp; d) : data(d), next(n) {};

        Node* getNext() const { return next; }

        E&amp; getData() { return data; }

        void setNext(Node* n) { next = n; }

    }; /** END NESTED NODE CLASS **/

    /** Begin fields of SinglyLinkedList class **/
    Node* head;
    Node* tail;
    int listSize;
    /** End fields of SinglyLinkedList class **/

public:

    SinglyLinkedList();
    ~SinglyLinkedList();

    //Accessor methods
    bool isEmpty() const;
    int size() const;
    const E&amp; first() const;
    const E&amp; last() const;

    //Update methods
    void addFirst(const E&amp;);
    void addLast(const E&amp;);
    E* removeFirst();

    void print() const;

}; /** END SINGLYLINKEDLIST CLASS HEADER **/

#include "SinglyLinkedList.tpp" //Include implementations of template code.

#endif /* LINKEDLISTS_SINGLYLINKEDLIST_H_ */



The implementations of the functions follow below: 

/*
 * SinglyLinkedList.tpp
 *
 *  Created on: Jan 5, 2020
 *      Author: dii
 */

/** Constructor **/
template&lt;typename E&gt;SinglyLinkedList&lt;E&gt;::SinglyLinkedList() : head(nullptr), tail(nullptr), listSize(0) {}

/** Destructor **/
template&lt;typename E&gt;SinglyLinkedList&lt;E&gt;::~SinglyLinkedList() {
    while(!isEmpty()) {
        delete removeFirst();
    }
}


/** Begin functions **/


template&lt;typename E&gt; bool SinglyLinkedList&lt;E&gt;::isEmpty() const { return listSize == 0; }

template&lt;typename E&gt; int SinglyLinkedList&lt;E&gt;::size() const { return listSize; }

template&lt;typename E&gt; const E&amp; SinglyLinkedList&lt;E&gt;::first() const { return head-&gt;getData(); }

template&lt;typename E&gt; const E&amp; SinglyLinkedList&lt;E&gt;::last() const { return tail-&gt;getData(); }

template&lt;typename E&gt; void SinglyLinkedList&lt;E&gt;::addFirst(const E&amp; d) {

    Node* temp = new Node(head-&gt;getNext(), d); //Create a new node.
    head = temp; // Set the head pointer to point to this object.
    listSize++; // Increase size of list by 1.

}

template&lt;typename E&gt; void SinglyLinkedList&lt;E&gt;::addLast(const E&amp; d) {

    Node* temp = new Node(nullptr, d);
    if(isEmpty()) {
        head = temp;
        tail = temp;
    } else {
        tail-&gt;setNext(temp);
        tail = temp;
    }

    listSize++;

}

template&lt;typename E&gt; E* SinglyLinkedList&lt;E&gt;::removeFirst() {

    if(isEmpty()) {
        return nullptr;
    }

    E* temp = &amp;(head-&gt;getData());
    head = head-&gt;getNext();
    listSize--;

    return temp;

}

template&lt;typename E&gt; void SinglyLinkedList&lt;E&gt;::print() const {
    Node* walk = head;
    while(walk != nullptr) {
        std::cout &lt;&lt; walk-&gt;getData() &lt;&lt; std::endl;
        walk = walk-&gt;getNext();
    }
}


I have a feeling that the destructor is not implemented as well as it could be. I am also still not entirely sure about when to use references, e.g., should the data field of the Node class be of type E&amp; or is just E ok?
Design

It's relatively simple to create a double linked list!



Some things you should think about some more:


Please put your code into a namespace. Do you think that SinglyLinkedList is a very unique name in the global scope?
You need to look into const correctness.
You need to look into move semantics.




Major Bug.

You have not implemented the rule of three (or five). When a class contains an owned pointer the compiler generated copy constructor, copy assignment operator and destructor are not going to work correctly.

Your SinglyLinkedList contains the owned pointer: head. An owned pointer is one that you have taken responsibility for the lifespan of (and are thus going to call delete on it when it the object is destroyed.

{
     SinglyLinkedList&lt;int&gt;    list1;
     list1.addFirst(1);

     SinglyLinkedList&lt;int&gt;    list2(list1);
}
// This code is broken as you have a double delete.




Upgrade you need to do.

The standard library defines things called concepts. A concept is a set of properties that a class has. One of the big concepts of the standard is the concept of a "Container" (of which a singly linked list is). Ao you should probably make your list "Container" compliant.

Code Review

I hate useless comments.

    /** BEGIN NESTED NODE CLASS **/
    class Node {


That counts as a useless comment.



OK good start.

        Node(Node* n, const E&amp; d) : data(d), next(n) {}   ;
                 // Not Needed                           ^^^


But it does mean that you are required to copy the object into the list. Modern C++ has the concept of move semantics. So you should allow for moving the object into the list.

        Node(Node* n, E&amp;&amp; d) : data(std::move(d)), next(n) {}


Also you should allow the object to be constructed in place:

        template&lt;typename... Args&gt;
        Node(Node* n, Args&amp;&amp;... args)
            : data(std::forward&lt;Args&gt;(args)...)
            , next(n)
        {}




Here you get a reference to the object.
That's fine.

        E&amp; getData() { return data; }


But what happens if your list has been passed by const reference to a function. Now you can no longer accesses the data as you can't get a const reference to the data.

        E const&amp; getData() const {return data;}


Its a bit strange that you only return reference from the node. But you return only const references from the list?

    const E&amp; first() const;
    const E&amp; last() const;




I actually hate your get/set mentatility here. I see no need for it inside the node object itself.

        void setNext(Node* n) { next = n; }




That's fine.

    void print() const;


But two things to note.


There is more streams than the std::cout so allow this to be pramertized.
The normal way of streaming is via the output operator: operator&lt;&lt;()

void print(std::ostream&amp; stream = std::cout);
friend std::ostream&amp; operator&lt;&lt;(std::ostream&amp; str, SinglyLinkedList const&amp; data)
{
    data.print(str);
    return str;
}