Generic List code with Iterator

I was wondering if this implementation of generic list with iterator is correct. The code compiles and goes fine. What would you think shall I improve? (first time I post here, I'm scared).
I'm also concerned about the const correctness of the code.

Class node

#ifndef LIST_H_
#define LIST_H_

template
class Node {
public:
    Node();
    Node(const Node& x);
    void setKey(const K& x);
    void setNext(Node * const & x);
    void setPrev(Node * const & x);
    Node* getNext() const;
    Node* getPrev() const;
    void insertNode(Node* const & nil);
    void removeNode(Node* const & nil);
    K getKey() const;
    K& getKeyRef();
private:
    K key;
    Node* next;
    Node* prev;
};

template
inline Node::Node() {
    this->setNext(this);
    this->setPrev(this);
}

template
inline Node::Node(const Node& x) {
    this->setKey(x.getKey());
    this->setPrev(x.getPrev());
    this->setNext(x.getNext());
}

template
inline void Node::setKey(const K& x) {
    this->key = x;
}

template
inline void Node::setNext(Node * const & x) {
    this->next = x;
}

template
inline void Node::setPrev(Node * const & x) {
    this->prev = x;
}

template
inline Node* Node::getNext() const {
    return this->next;
}

template
inline Node* Node::getPrev() const {
    return this->prev;
}

template
inline K Node::getKey() const {
    return this->key;
}

template
inline K& Node::getKeyRef() {
    return this->key;
}

template
inline void Node::insertNode(Node* const & nil) {
    this->setPrev(nil->getPrev());
    this->setNext(nil);
    nil->getPrev()->setNext(this);
    nil->setPrev(this);
}

template
inline void Node::removeNode(Node* const & nil) {
    Node *prev, *next;
    if(this != nil) {
        prev = this->getPrev();
        next = this->getNext();
        prev->setNext(next);
        next->setPrev(prev);
    }
}

#endif /* LIST_H_ */

Class list with nested iterator

```
#ifndef LIST_LIST_H_
#define LIST_LIST_H_

#include "list_node.h"
#include

template
class List

Here are some things that may help you improve your code.

Use consistent naming

The include guard says #ifndef LIST_H but the actual include says #include "list_node.h". It's not technically an error, but I'd suggest that changing the include guard to #ifndef LIST_NODE_H_.

Fix the destructor

Right now there is a memory leak in the destructor. You need to add delete nil; as the last line.

Don't expose class internals

The getKeyRef() is a dangerous function because it essentially returns a handle to an internal data member of Node. In fact, because all of the private data members have functions which "leak" their internals, I'd suggest making Node a struct and then making it a private within the List class.

Avoid cluttering the code with this->

All of the instances of this-> are just visual noise and don't really add anything to the program. I'd recommend omitting all such instances.

Eliminate unneeded functions

The getSentinel() routine, it seems to me, is probably only useful within the List class, so I'd recommend eliminating the routine and simply using the sentinel member variable instead. Similarly, Iterator::setCurrent() and Iterator::getCurr() seem unnecessary. I'd also recommend omitting ~Iterator() since it literally does nothing.

Rethink variable names

The variables next and prev and key are good names because they suggest how they are used. However, nil and sentinel are not as good, in my view. In some languages nil is the equivalent to nullptr and it seems to me that instead of sentinel, a better name might be root. Also, I'd suggest that having root be a default-constructed Node instead of a pointer would simplify this class.

Consider a different constructor

The Node class could benefit from a constructor like this:

Node(const T& value, Node *prev_=nullptr, Node *next_=nullptr) : 
    key{value}, prev{prev_}, next{next_} 
{}

Simplify the insert code

Removing the functions from Node and making it a struct internal to List allows a much simplified insert routine. For example, here's how the Node might look:

template
struct Node {
    Node(const T& value) :
        key{value}, prev{this}, next{this} 
    {}
    Node(const T& value, Node *p, Node *n) :
        key{value}, prev{p}, next{n} 
    {}
    T key;
    Node *prev;
    Node *next;
};

This allows the insert routine to be written like this:

template
inline void List::insert(const K& x) {
    auto tail = root.prev;
    root.prev = tail->next = new Node(x, tail, &root);
}

This also assumes that the previous few changes have also been made.

Use nullptr rather than 0

Instead of this->setCurrent(0);, it would be better to use setCurrent(nullptr); or simply curr = nullptr;. While they result in the same thing, using nullptr makes the context and intent much more clear.

Consider implementing a bidirectional iterator

Since your List is a doubly-linked list, it should be relatively simple to provide a bidirectional iterator instead of just the forward iterator that is currently implemented.

Consider adding a construct that uses std::initializer_list

It would be nice to be able to instantiate a list like this:

List mylist{"alpha", "beta", "gamma", "delta"};

This can be relatively easily added using the C++11 std::initializer_list.

StackExchange Code Review Q#158666, answer score: 2