# Generic Single Linked List using smart pointers

I have decided to rewrite what I did here, following the suggestions to use smart pointers. I will rewrite the other data structures as well using smart pointers where appropriate.

I just want to see how my code stands now, I am sure there are still areas I need to improve or fix. I again want to thank this community in their effort in evaluating my code, I really appreciate it and I believe it is slowly but surely taking my coding skills to the next level.

#ifndef SingleLinkedList_h

#include <iostream>

template <class T>
private:

struct Node {
T data;
std::unique_ptr<Node> next = nullptr;
Node(T x) : data(x), next(nullptr) {}
};
Node* tail = nullptr;

// This function is for the overloaded operator <<
void display(std::ostream &str) const {
for (Node* loop = head.get(); loop != nullptr; loop = loop->next.get()) {
str << loop->data << "\t";
}
str << "\n";
}

public:
// Constructors
SingleLinkedList() = default;                                           // empty constructor

// Rule of 5

friend std::ostream& operator<<(std::ostream &str, SingleLinkedList &data) {
data.display(str);
return str;
}

// Memeber functions
bool empty() const { return head.get() == nullptr; }
int getSize() const;
void push(const T &theData);
void push(T &&theData);
void display() const;
void insertTail(const T &theData);
void insertPosition(int pos, const T &theData);
void deleteTail();
void deleteSpecific(int delValue);
bool search(const T &x);
};

template <class T>
for(Node* loop = source.head.get(); loop != nullptr; loop = loop->next.get()) {
push(loop->data);
}
}

template <class T>
move.swap(*this);
}

template <class T>
move.swap(*this);
return *this;
}

template <class T>
}
}

template <class T>
swap(copy);
return *this;
}

template <class T>
using std::swap;
swap(tail, other.tail);
}

template <class T>
int size = 0;
for (auto current = head.get(); current != nullptr; current = current->next.get()) {
size++;
}
return size;
}

template <class T>
std::unique_ptr<Node> newNode = std::make_unique<Node>(theData);

}

else {
tail->next = std::move(newNode);
tail = tail->next.get();
}
}

template <class T>
std::unique_ptr<Node> newnode = std::make_unique<Node>(std::move(thedata));

}

else {
tail->next = std::move(newnode);
tail = tail->next.get();
}
}

template <class T>
while (newNode != nullptr) {
std::cout << newNode->data << "\t";
newNode = newNode->next;
}
}

template <class T>
std::unique_ptr<Node> newNode = std::make_unique<Node>(theData);
}

template <class T>
std::unique_ptr<Node> newNode = std::make_unique<Node>(theData);
tail->next = std::move(newNode);
tail = tail->next.get();
}

template <class T>
void SingleLinkedList<T>::insertPosition(int pos, const T &theData) {
if (pos > getSize() || pos < 0) {
throw std::out_of_range("The insert location is invalid.");
}
int i = 0;

for (; node && node->next && i < pos; node = node->next.get(), i++);

if (i != pos) {
throw std::out_of_range("Parameter 'pos' is out of range.");
}

auto newNode = std::make_unique<Node>(theData);

if (node) {
newNode->next = std::move(node->next);
node->next = std::move(newNode);
}
else {
}
}

template <class T>
throw std::out_of_range("List is Empty!!! Deletion is not possible.");
}

auto next = std::move(current->next);
}

template <class T>
throw std::out_of_range("List is Empty!!! Deletion is not possible.");
}

Node* previous = nullptr;

while (current->next != nullptr) {
previous = current;
current = current->next.get();
}
tail = previous;
previous->next = nullptr;
}

template <class T>

throw std::out_of_range("List is Empty!!! Deletion is not possible.");
}

Node* temp2 = nullptr;
while (temp1->data != delValue) {
if (temp1->next == nullptr) {
}
temp2 = temp1;
temp1 = temp1->next.get();
}
temp2->next = std::move(temp1->next);
}

template <class T>
while (current != nullptr) {
if (current->data == x) {
return true;
}
current = current->next.get();
}
return false;
}



Here is the main.cpp file:

#include <algorithm>
#include <cassert>
#include <iostream>
#include <ostream>
#include <iosfwd>

int main(int argc, const char * argv[]) {

///////////////////////////////////////////////////////////////////////
///////////////////////////////////////////////////////////////////////
obj.push(2);
obj.push(4);
obj.push(6);
obj.push(8);
obj.push(10);
std::cout<<"\n--------------------------------------------------\n";
std::cout<<"---------------displaying all nodes---------------";
std::cout<<"\n--------------------------------------------------\n";
std::cout << obj << std::endl;

std::cout<<"\n--------------------------------------------------\n";
std::cout<<"-----------------Inserting At End-----------------";
std::cout<<"\n--------------------------------------------------\n";
obj.insertTail(20);
std::cout << obj << std::endl;

std::cout<<"\n--------------------------------------------------\n";
std::cout<<"----------------Inserting At Start----------------";
std::cout<<"\n--------------------------------------------------\n";
std::cout << obj << std::endl;

std::cout<<"\n--------------------------------------------------\n";
std::cout<<"-------------inserting at particular--------------";
std::cout<<"\n--------------------------------------------------\n";
obj.insertPosition(5,60);
std::cout << obj << std::endl;

std::cout << "\n--------------------------------------------------\n";
std::cout << "-------------Get current size ---=--------------------";
std::cout << "\n--------------------------------------------------\n";
std::cout << obj.getSize() << std::endl;

std::cout<<"\n--------------------------------------------------\n";
std::cout<<"----------------deleting at start-----------------";
std::cout<<"\n--------------------------------------------------\n";
std::cout << obj << std::endl;

std::cout<<"\n--------------------------------------------------\n";
std::cout<<"----------------deleting at end-----------------------";
std::cout<<"\n--------------------------------------------------\n";
obj.deleteTail();
std::cout << obj << std::endl;

std::cout<<"\n--------------------------------------------------\n";
std::cout<<"--------------Deleting At Particular--------------";
std::cout<<"\n--------------------------------------------------\n";
obj.deleteSpecific(4);
std::cout << obj << std::endl;
std::cout << std::endl;

obj.search(8) ? printf("yes"):printf("no");

std::cout << "\n--------------------------------------------------\n";
std::cout << "--------------Testing copy----------------------------";
std::cout << "\n--------------------------------------------------\n";
std::cout << obj1 << std::endl;

//std::cout << "\n-------------------------------------------------------------------------\n";
//std::cout << "--------------Testing to insert in an empty list----------------------------";
//std::cout << "\n-------------------------------------------------------------------------\n";
//obj2.insertPosition(5, 60);
//std::cout << obj2 << std::endl;

std::cin.get();
}


1. You are including too little. std::unique_ptr needs <memory>, std::swap needs <utility>, std::invalid_argument needs <stdexcept>.

2. Defining and using a ctor for your Node instead of using aggregate-initialization forces you to make unnecessary, maybe costly or even impossible, copies. Considering your interface, that's very surprising for your clients.

Remove all ctors from Node, and use aggregate-initialization instead.

3. I honestly have no idea why you defined the private .display(std::ostream&). Just put it back into friend std::ostream& operator<<(std::ostream&, SingleLinkedList const&).

4. Defining .display() needlessly chains your code to std::cout. Otherwise, you could include <ostream> instead of <iostream> and avoid initializing all the C++ streams.

5. If you want to stream a single character, don't stream a string. Needless inefficiency is bad.

6. Users expect an iterator-interface, and it would make copying, printing, as well as inserting and deleting at specific points easier and/or more efficient. Enabling use of standard algorithms is also nice.

You should implement ForwardIterators.

7. Your names are outlandish (.getSize() => .size(), .push() / insertTail() => .push_back(), .insertHead() => .push_front(), .deleteHead() => .pop_front(), .deleteTail() => .pop_back()). That means no standard algorithms to you, and it's much harder to work with.

8. I would expect .search() to return an iterator. As you don't have any, maybe a pointer. But certainly not a bool, that's what .contains() would be for.

9. You don't allow construction from std::initializer_list<T>, nor an Iterator-pair. That's disappointing.

10. You know pointer != nullptr is the same as pointer in a boolean context? The same for pointer == nullptr and !pointer.

11. Avoid mixing iostreams and stdio without good reason. Also, only use std::endl if you need an explicit flush. But, maybe you do...

12. You know you can fuse two string-literals simply by not separating them with anything but whitespace, including newlines?

• Could you explain more for number 2 on what I should do to use aggregate initialization for my Node? – Snorrlaxxx Aug 4 '18 at 21:10
• For point number 4, should I just remove the .display() function entirely? – Snorrlaxxx Aug 4 '18 at 21:13
• Also, how would I go about applying your point number 6? Do you have any suggestions or something I can refer to? – Snorrlaxxx Aug 4 '18 at 21:14
• @Snorrlaxxx: 2. Elaborated. 4. Yes. 6. Linked to the Iterator-type you should write. – Deduplicator Aug 5 '18 at 10:38
• I am sorry I still do not understand what I need to do for number 2. Also, I have no idea how to implement number 6. – Snorrlaxxx Aug 5 '18 at 16:52

Your header file shouldn't require other headers to be included first. You are missing includes of <memory> and <utility> for it to be usable. You can help yourself spot such omissions by including your own headers first, before the standard library headers, in your main.cpp.

It helps to be consistent with standard collections - this consistency is useful, as it allows your collection to be used in a generic (template) function. For this reason, I suggest size_t size() const instead of int getSize() const.

Instead of search(), it might be better to implement iterators for your collection. Then we have the full range of <algorithm> functions, including find_if() and so on. We'd be able to replace deleteSpecific() with an erase() method that accepts an iterator or two iterators, more like the standard collections. (This erase() would then usefully provide you with a clear() method, and you could use that in the destructor to avoid unlimited recursion in the cleanup).

What is the purpose of the no-argument display()? It seems to just duplicate the streaming operator, but constrained to standard output, rather than being usable with std::clog or to a file stream, for example. And the other, private display() is used only in one place, so perhaps it should be inlined within the streaming operator.

• Could you go into more detail how I would improve this class with the use of iterators and such? I am a bit unfamiliar with their implementation. – Snorrlaxxx Aug 4 '18 at 21:20
• How to create iterators is an entire answer in itself! – Toby Speight Aug 6 '18 at 8:32
#ifndef SingleLinkedList_h


Most naming conventions reserve UPPERCASE to differentiate between preprocessor (Macro) variables from C++ language constructs/variables.

The name of your include guard should aim to minimize the chance of clashes with names from other code. Use differentiators like project name, physical path, logical path, author name, date, and/or GUIDs.

// Exposition-only


#include <iostream>


Do not include <iostream> in header files. Most C++ implementations will inject static constructors into every translation unit that includes your header, whether or not IO facilities are needed.

    struct Node {
T data;
std::unique_ptr<Node> next = nullptr;
Node(T x) : data(x), next(nullptr) {}
};


std::unique_ptr<> requires <memory>.

Constructors with a single non-default parameter should be declared with the function specifier explicit to avoid unintended implicit conversions.

A constructor should create a fully initialized object. If I have both the data and the next pointer, I should be able to create an object without requiring the extra step of assignment to next. What if I want to construct data in place? Is T guaranteed to be cheap to copy?

Update

    // Exposition-only
struct Node {
T data;
std::unique_ptr<Node> next = nullptr;

// disable if noncopyable<T> for cleaner error msgs
explicit Node(const T& x, std::unique_ptr<Node>&& p = nullptr)
: data(x)
, next(std::move(p)) {}

// disable if nonmovable<T> for cleaner error msgs
explicit Node(T&& x, std::unique_ptr<Node>&& p = nullptr)
: data(std::move(x))
, next(std::move(p)) {}
};


    void display(std::ostream &str) const {
for (Node* loop = head.get(); loop != nullptr; loop = loop->next.get()) {
str << loop->data << "\t";
}
str << "\n";
}


This is one of your dependents on <iostream> that isn't necessary. A better approach is to create an interface to your data. In the standard library, that is done through iterators. You can also apply a visitor function to each element.

// Exposition-only
public:
template <typename UnaryOp>
void for_each_element(UnaryOp f) const {
std::invoke(f, std::as_const(current->data));
}
}


display, display(str), and operator<< can now be standalone functions. They can be placed in their own header and the user can selectively include it into their project if they really want output facilities.

    bool empty() const { return head.get() == nullptr; }


std::unique_ptr<>::operator bool() exists. You neither have to call get() or compare to nullptr.

template <class T>
for(Node* loop = source.head.get(); loop != nullptr; loop = loop->next.get()) {
push(loop->data);
}
}


Seems like a good use case for a visitor traverser.

Update

// Exposition-only
template <class T>
source.for_each_element([&](auto&& value) { push(value); });
}


    template <class T>
}
}


The body of your destructor would be useful to the user if they ever wanted to clear() their list.

Think semantically, not just syntactically. You check to see if the list is empty here, but you also check again in deleteHead().

    // Exposition-only
private:
template <class T>
}

public:
template <class T>
clear();
}

template <class T>
do_pop_front();
}
}

template <class T>
if (empty()) { throw ... }
do_pop_front();
}


     using std::swap;


Requires <utility>.

template <class T>
int size = 0;
for (auto current = head.get(); current != nullptr; current = current->next.get()) {
size++;
}
return size;
}


If you want your container to be usable with existing standard interfaces, follow the naming conventions (size). You can read about the Container requirements.

Consider caching the size as a data member. It's simple$^{1}$ for your class to keep count as operations on your list happen. Note - Splicing would become an $O(n)$ operation as it has to recalculate the size on splice.

Should int represent your size type?

$^{1}$"There are 2 hard problems in computer science: cache invalidation, naming things, and off-by-1 errors." -- Leon Bambrick

template <class T>
std::unique_ptr<Node> newNode = std::make_unique<Node>(theData);

}

else {
tail->next = std::move(newNode);
tail = tail->next.get();
}
}


std::move requires <utility>. The logic can be simplified.

// Exposition-only.
template <class T>
std::unique_ptr<Node> newNode = std::make_unique<Node>(theData);
newNode->next = std::move(head); // new node takes ownership of existing nodes
}


template <class T>
std::unique_ptr<Node> newNode = std::make_unique<Node>(theData);
tail->next = std::move(newNode);
tail = tail->next.get();
}


Singly-linked lists typically operate from one-end. This leads to unusual behavior where insertion at the front or back is constant, but deletion at the front is constant while the back is linear (as tail needs to be updated).

template <class T>
void SingleLinkedList<T>::insertPosition(int pos, const T &theData) {
if (pos > getSize() || pos < 0) {
throw std::out_of_range("The insert location is invalid.");
}


std::out_of_range requires <stdexcept>. Is pos == getSize() a valid state?

    auto node = head.get();
int i = 0;

for (; node && node->next && i < pos; node = node->next.get(), i++);


This could be a helper function that advances a node by $pos$ elements.

    if (i != pos) {
throw std::out_of_range("Parameter 'pos' is out of range.");
}


Why would this ever be true if we know pos is in $[0, getSize())$?

    auto newNode = std::make_unique<Node>(theData);

if (node) {
newNode->next = std::move(node->next);
node->next = std::move(newNode);
}
else {
}


So we're not inserting at the position but after the position. Perhaps the function should be named insert_after?

template <class T>
...
Node* previous = nullptr;

while (current->next != nullptr) {
previous = current;
current = current->next.get();
}


Abstracting this into an adjacent find helper could be useful for this function, and deleteSpecific().

    tail = previous;
previous->next = nullptr;


If you decide to allow operating on both ends of the list, I would simply this function. deleteHead was simplified in the comment on the destructor.

// Exposition-only
template <class T>
tail.reset();
}

template <class T>
/* throw on empty... */
while (true) {
if (!current) {
return; // Why throw?
}
current->next = std::move(current->next->next);
current = current->next.get();
}
}


#include <algorithm>
#include <cassert>
#include <iostream>
#include <ostream>
#include <iosfwd>


Organize your headers. If you order them in logical groups - from implementation level to language level to system level - you can catch latent usage errors in your own code as soon as possible.

1. Prototype/Interface headers - matching .hpp that corresponds to a .cpp
3. Non-standard non-system headers - boost, eigen
4. Standard headers - iostream, cstdint, vector
5. System headers - May require configuration and out of order use because of dependencies.

Order each of those logical subgroups by name (if possible) to allow readers to quickly parse through longer lists.

You don't need <algorithm>, <cassert>, <ostream>, or <iosfwd>.

int main(int argc, const char * argv[]) {


If you are not accepting command-line arguments, you can omit the parameters for main. (int main() {).

    SingleLinkedList<int> obj;
obj.push(2);
obj.push(4);
obj.push(6);
obj.push(8);
obj.push(10);
std::cout<<"---------------displaying all nodes---------------";
std::cout << obj << std::endl;


Rather than memorizing the 25 or so operations, in-order, and how the internal state flows, just use a testing framework (Catch2). Reduce your cognitive load, test more, and process useful information (what failed?).

// Exposition-only
#define CATCH_CONFIG_MAIN

#include "catch2/catch.hpp"

TEST_CASE( "single linked list operations", "[slist]" ) {
for (int i = 2; i < 11; i += 2) {
obj.push(i);
}

SECTION( "get current size" ) {
REQUIRE(obj.getSize() == 5);
}
...
SECTION( "inserting at end" ) {
obj.insertTail(20);
REQUIRE( obj.size() == 6 );
// REQUIRE( obj.back() == 20 );
}
...
SECTION( "empty list throws on delete head" ) {
obj.clear();
REQUIRE( obj.empty() );
}
...
}

• Thank you for the comments. I am still having trouble implementing all of what you suggested. Could you provide a solution to what you intend for me to change using my code? – Snorrlaxxx Aug 5 '18 at 17:14
• Look at your implementation's <forward_list>. – Snowhawk Aug 5 '18 at 21:09
• what do you mean my implementation's <forward_list> I do not understand? – Snorrlaxxx Aug 5 '18 at 21:36
• What exactly are you having problems implementing? – Snowhawk Aug 5 '18 at 21:39
• Well, iterators and some of the other suggestions you had involving the constructor. – Snorrlaxxx Aug 5 '18 at 21:41

# Create iterators over the elements in the list

Here's how to go about creating iterators for this collection. They will naturally be forward iterators, as we can't traverse the singly-linked list backwards.

First, let's declare the iterator class, and begin() and end() methods to create them:

public:
class iterator;
iterator begin();
iterator end();


The state within an iterator is obviously its current Node:

template<class T>
{
Node* node;
}


For a forward iterator, we need to be able to dereference and to increment it; also we need to be able to compare iterators for equality, and to construct (including default-construct) instances:

template<class T>
{
Node* node = nullptr;

public:
iterator(Node *node = nullptr)
: node(node)
{}

bool operator!=(const iterator& other) const { return node != other.node; }
bool operator==(const iterator& other) const { return node == other.node; }

T& operator*() const { return node->data; }
T& operator->() const { return node->data; }

iterator& operator++() { node = node->next.get(); return *this; }
};

template<class T>
{
}

template<class T>
{
return {};
}


To use with standard algorithms, we need to provide some type names that can be found by std::iterator_traits:

#include <iterator>

template<class T>
{
public:
using iterator_category = std::forward_iterator_tag;
using value_type = T;
using difference_type = std::ptrdiff_t;
using pointer = T*;
using reference = T&;


Then we can use iterators to simplify our code:

template <class T>
return std::find(begin(), end(), x) != end();
}


## Exercise

Implement a const iterator for the list. You'll start with these methods in SingleLinkedList:

class const_iterator;
const_iterator cbegin() const;
const_iterator cend() const;
const_iterator begin() const;
const_iterator end() const;


and the iterator traits will refer to const T in place of T.

• You might find it easier to user your iterators for erase() and such if you make the state be a pointer to the smart pointer that points to the node. I.e. class iterator { std::unique_ptr<Node>* current; }. Experiment with both, and see what their relative advantages are! – Toby Speight Aug 6 '18 at 8:35
• Thank you, I am going to implement what you suggested, appreciate the help immensely. – Snorrlaxxx Aug 7 '18 at 0:00
• Will I also include the using iterator_category, etc in the const_iterator class as well? – Snorrlaxxx Aug 7 '18 at 0:15
• Yes, you'll need that. You'll find a lot of duplication between the mutable and const iterators. That can be reduced using templates, but I recommend you focus on understanding the iterators and working out how to use them in your methods before getting aggressive with the DRY principle. Oh, and do come back for a review of your iterators when you think it's ready! – Toby Speight Aug 7 '18 at 8:05
• Hi Toby, thank you. I will come back for sure. That being said, I was thinking of once I believe I am done with single linked list I would start on re-writing double linked list and incorporate what I learned in the single linked list. That way if there are still things I am getting wrong or not doing then I will know I need to change some of those in my single linked list code as well, does that sense? – Snorrlaxxx Aug 8 '18 at 0:16