Linked list with iterators

This is my second linked list implementation for review. I rewritten pretty much all of this and have added iterators, sentinels and The Rule of Five.

Please let me know what's wrong with it; I am still not confident in many areas.

```
#include
#include

#pragma once

template
class LinkedList;

template
class LinkedListIterator
{
friend class LinkedList;
TNode* p;
public:
LinkedListIterator(TNode* p) : p(p) {}
LinkedListIterator(const LinkedListIterator& other) : p(other.p) {}
LinkedListIterator& operator=(LinkedListIterator other) { std::swap(p, other.p); return *this; }
void operator++() { p = p->next; }
void operator++(int) { p = p->next; }
bool operator==(const LinkedListIterator& other) { return p == other.p; }
bool operator!=(const LinkedListIterator& other) { return p != other.p; }
const int& operator*() const { return p->data; }
LinkedListIterator operator+(int i)
{
LinkedListIterator iter = *this;
while (i-- > 0 && iter.p)
{
++iter;
}
return iter;
}
};

template
class Node
{
friend class LinkedList;
friend class LinkedListIterator>;
friend class LinkedListIterator>;

Node() : next(nullptr) {}
Node(const T &data) : data(data), next(nullptr) {}
Node *next;
T data;
public:
typedef T value_type;
};

template
class LinkedList
{
typedef Node node;

std::size_t size;
std::unique_ptr head;
std::unique_ptr tail;

void init()
{
size = 0;
head.reset(new node);
tail.reset(new node);
head->next = tail.get();
}

public:
typedef LinkedListIterator iterator;
typedef LinkedListIterator const_iterator;

LinkedList() { init(); }

LinkedList(const LinkedList& other)
{
init();
const_iterator i = other.begin();
while (i != other.end())
{
add(*i);
i++;
}

head.reset(other.head.get());

In all, this is pretty well structured code and I had no problem reading and understanding it. Whenever another programmer can read and understand your code, it's a good sign that you're on the right track. So what's left is mostly small points that might improve your code. Here's what I noticed in no particular order.

Use the appropriate #includes

This code uses std::swap which is actually defined in ` up to C++11, but in more recent versions of the standard. You've included but in a C++ program that should actually be which puts the various declarations into the std:: namespace rather than in the global namespace.

Use the right forms of const

The LinkedList class contains a getSize() function. Right now that's declared as

const int getSize()    // const will be ignored for this return type

but I think what you really meant was this:

int getSize() const     // this call does not alter the underlying object

Don't use variables that are not in scope

The two move operators of the LinkedList class both refer to a member data pointer named first but no such variable is actually declared. The three lines in those two functions that refer to first should simply be deleted.

Think about temporary object usage

The LinkedList class includes a number of member functions in which remove(begin()) is called. However, a close look shows that begin() returns a temporary of type iterator. However, the remove() function takes a non-const reference to an iterator and so we have a problem. One solution is to change remove to use move semantics.

Another problem is easily shown when using this common C++11 idiom to print all of the data members of the linked list:

for (const auto &val : mylist)
    std::cout << val << '\n';

The intent here is to print the values of all of the linked list values without modifying them (hence const) and without copying them (hence &). Unfortunately this won't work with the current version of the code. The problem is once again the use of temporary values. In particular, this effectively calls this operator:

const int& operator*() const { return p->data; }

But p->data isn't necessarily an int so the way to fix this is:

typename TNode::value_type& operator*() const { return p->data; }

Write member initializers in declaration order

The Node class has this constructor

Node(const T &data) : data(data), next(nullptr) {}

That looks fine, but in fact, next will be initialized before data because members are always initialized in declaration order and next is declared before data in this class. To avoid misleading another programmer, you should swap the order of those such that it says instead:

Node(const T &data) : next(nullptr), data(data) {}

This way the initialization actually proceeds from left to right as one might expect at first glance.

Iterator increment operators should return a reference

The code has two increment operators, a preincrement and a postincrement:

void operator++() { p = p->next; }
void operator++(int) { p = p->next; }

However, consider the following use:

for (auto it = ll.begin(); it != ll.end(); )
    std::cout << *it++ << '\n';

This will fail because it++ returns void instead of a reference to a LinkedListIterator. The preincrement is easy to fix:

LinkedListIterator& operator++() { p = p->next; return *this; }

The postincrement cannot be the same thing, though, because it is required to return the value before the increment. In other words, if int i = 5; we would expect std::cout << ++i << '\n'; to print "6" but std::cout << i++ << '\n'; should print "5". So to fix the postincrement we do this:

LinkedListIterator operator++(int) { LinkedListIterator it(*this); p = p->next; return it; }

Note that this returns an actual LinkedListIterator object and not an object reference as with the preincrement operator.

Consider making the iterators more general purpose

As it stands, an attempt to sort an instance of this LinkedList using std::sort will fail:

std::sort(ll.begin(), ll.end());

The problem is that std::sort expects to be able to check iterator traits (such as random_access_iterator_tag) to tell the Standard Template Library (STL) which iterators can support which algorithms.

Use cbegin and cend

The STL uses cbegin and cend as names for the constant versions of the iterator members. This is convenient because it allows usage such as this:

for (auto it = ll.cbegin(); it != ll.cend(); ++it)
    // do something with each member

This code can easily be modified to conform to this by simply renaming the two functions.

Fix your copy constructors

This code will cause a seg fault:

``
#include
int main()
{
LinkedList ll;
ll.add("one");
LinkedList l2 = ll;
l2.add("two");
std::cout << "The copy\n";
for (auto it = l

StackExchange Code Review Q#55834, answer score: 9