Single LinkedList using smart pointers

In order to learn C++11, I propose to myself implement a simple single linked list using smart pointers. The idea was to cover the basic operations without leave any memory leak.

I came up with a solution but I'm sure that it could be improved in many ways. So, I would like to receive feedback of any kind: format, readability, implementation, etc.

Also, I would like to hear your opinion about which is the correct way of iterate over a sequence of objects using smart pointers. After reading and looking for examples, I believe that it should be done using the get method (from the smart pointer) to obtain a raw pointer, which can be used to go through the sequence. Is there another way of doing it?.

And finally, what else could I add to the class or what do you think that is missing that I should add? I will use the answer for this question as an opportunity to keep improving/learning, so every answer is welcome.

```
#ifndef LINKED_LIST_H
#define LINKED_LIST_H

#include
#include

template
class LinkedList
{
private:
/ Node definition /
class Node
{
friend class LinkedList;
private:
const T key_;
std::unique_ptr next_;
public:
Node(const T k): key_{k}, next_{nullptr} {}
~Node() = default;
// Commented lines: just for debuging purposes.
// Node(const T k): key_{k}, next_{nullptr} { std::cout head_;

public:
LinkedList() = default;
~LinkedList() = default;
//~LinkedList(){ std::cout
T LinkedList::at(unsigned int index)
{
auto ptr = head_.get();
while (--index)
ptr = ptr->next_.get();
return ptr->key_;
}

template
bool LinkedList::is_empty() const
{
return head_ == nullptr;
}

template
T LinkedList::pop_back()
{
// This method does not handle the empty list case.
auto prev_ptr = head_.get();
auto curr_ptr = prev_ptr;
while (curr_ptr->next_)
{
prev_ptr = curr_ptr;
curr_ptr = curr_ptr->next_.get();
}

Overall, it looks pretty good. A few design point I'd like to add:

-
Have you considered making it an intrusive list instead? There isn't much use for your list class, besides the learning process of writing one, when the standard provides std::list and std::forward_list templates, however, a templated intrusive list class can have quite a few uses, and such class is not provided by the standard (thought Boost do provide an intrusive list template). Nevertheless, if you'd like to read on the topic, here are a couple references from Google:

-
A StackOverflow question.

-
Intrusive data structures in the context of game development.

-
Your push_back() method could be made to run in constant time if you keep an extra reference to the list's tail. Right now, any insertion after the first one will require a full traversal of the list to find the tail. If instead you keep a valid reference to the current list tail at every new insertion, then the loop can be replaced by a cheap pointer swapping.

-
Since Node is already an internal private class inside LinkedList, it seems a bit of overkill making it a class and then friending list with it. Node could very well be a struct or a class with all fields public.

Looking at the current code you have, there are a few minor details that stand out to me:

-
You are keeping track of the list size/item-count, but the interface is missing a method to query its current size/count.

-
You should explicitly initialize size_ to zero and head_ to null either directly in the declaration (allowed by C++11) or in the constructor's initializer list. Don't leave room for misunderstandings. Some things are better if explicit.

-
Your at() method doesn't do bounds checking. If index goes beyond the size_, you will eventually try to access a null pointer and crash. You should at least validate that index is

-
Since front(), back() and at() are returning copies of the stored values, you should also mark them with const. print() is also just reading data from the structure, so it is naturally a const method as well. By the way, you didn't provide an implementation for front() and back(). Even tough I can have an idea about how they'd look...

-
Instead of returning by value in methods like at() & friends and always creating a copy of the stored item, consider providing two versions of the methods, one returning a mutable reference (T&) and one returning a const reference (const T&). This can be an important optimization when storing large objects. Many times the caller only wants to look at a value, so creating a copy all the time can be detrimental to performance. Also, by always returning by value, a method like at() cannot be used to mutate the stored item. E.g. something like list_of_ints.at(5) = 42; would not change the value stored at 5. If you intend to allow this idiom, then an overload returning by reference is necessary.

-
Like you have noted in the comments, pop_front() and pop_back() will crash for an empty list. I would still recommend at least an assert to help debugging. Remember that assertions can be disabled, so they will cost you nothing if you disable them. Use a runtime exception if you prefere having the check always there.

-
I'd contend that the C++ world is divided between programmers that prefer printing things with a plain function/method call and those that prefer to use the stream operators. You should provide both to appease all the factions ;). The stream operator can be implemented in terms of print() so it will only be a couple extra lines of code.

-
Instead of having commented-out code for your debug prints, you could define a simple wrapper macro for cout that can be disabled/enabled via your build system, without having to change the source code. This is one of the legitimate uses for a macro in C++, IMHO.

#define MY_DEBUG_LOG(message) do { std::cout << "[DEBUG]: " << message << "\n"; } while (0)

-
Lastly, you should get a StackExchange badge just for not using namespace std; in your code :P.

StackExchange Code Review Q#90204, answer score: 4