LinkedList with Node implementation

Below is my implementation of templated Node and LinkedList classes. I would really appreciate it if someone would give me pointers on what I can improve.

```
//LinkedList with SumLists()

#include
#include

using namespace std;

template
class Node
{
public:
T data;
Node * next;
Node(const T& d):data(d), next() {}
Node(const Node& copyNode) : data(copyNode.data), next() {}

private:
Node& operator=(const Node&);
};

template
class LinkedList
{
public:

Node * head;
Node * tail;

LinkedList(const LinkedList& LL);
LinkedList& operator=(LinkedList byValList);
LinkedList(): head(NULL), tail(NULL) {}
LinkedList(Node * newNode) : head(newNode), tail(newNode) {}
~LinkedList();

static LinkedList sumLists(const LinkedList& LL1, LinkedList& LL2);

void insertToTail(T val);
void insertToHead(T val);
void print();
void printBackwards();
};

template
LinkedList::LinkedList(const LinkedList& LL) : head(NULL), tail(NULL)
{
const Node * curr = LL.head;

if (!head && curr)
{
head = new Node(curr->data);
tail = head;
curr = curr->next;
}

while (curr)
{
Node * newNode = new Node(curr->data);
tail->next = newNode;
tail = newNode;
curr = curr->next;
}
}

template
LinkedList& LinkedList::operator=(LinkedList byValList)
{
std::swap(head, byValList.head);
return *this;
}

template
LinkedList::~LinkedList()
{
Node * curr = head;
while (head)
{
head = head->next;
delete curr;
curr = head;
}
}

template
void LinkedList::insertToTail(T val)
{
Node * newNode = new Node(val);
if (tail == NULL)
{
newNode->next = tail;
tail = newNode;
head = newNode;
return;
}
tail->next = newNode;
tail = tail->next;
}

template
void LinkedList::insertToHead(T val)
{
Node * newNode = new Node(val);
newNode->next = head;
head = newNode;
i

Please. Oh please stop doing this.

using namespace std;

If this was a header file you just polluted the global namespace for anybody that uses your file. This will get it banned from any serious project. This is done in textbooks for some reason and is fine for short ten line example programs. But once you get past 10 lines it has issues. Stop using it; it is a bad habbit that will get you into real problems on any decent sized project.

Why is “using namespace std;” considered bad practice?

You do really the standard library is in the namespace std. So it only costs you 5 extra characters to use it.

std::list    myList;

Node is an implementation detail of the list. There is no reason for anybody using the list to know exactly how you implemented. Nor is there a reason to provide them with a Node class (as you will now need to maintain that concept).

template
class Node
{
public:
    T data;
    Node * next;
    Node(const T& d):data(d), next() {}
    Node(const Node& copyNode) : data(copyNode.data), next() {}

private:
    Node& operator=(const Node&);
};

So I would make Node a private member of LinkedList.

Its not really a copy constructor if you don't copy the next member.

Node(const Node& copyNode) : data(copyNode.data), next() {}

But OK. I can see this as an optimization.
But personally I would have used a third constructor.

Node(const Node& copyNode, Node* next)
    : data(copyNode.data)
    , next(next) 
{}

Then you can pass NULL as the second parameter, to initialize next.

So you have disabled the assignment operator:

private:
    Node& operator=(const Node&);

This is correct in C++03. But this is 2014 and C++11 is supported by all modern compilers and most already support C++14. So you should start using the modern version of the language.

Node& operator=(const Node&) = delete;

Your implementation of linked list uses NULL as a terminator (which is fine). But if you add a fake sentinel value to your list it makes the implementation much easier as you never have NULL pointers (and end points at the sentinel).

In the copy constructor:

if (!head && curr)

At this point head is always NULL. You just set it two lines above.

The other thing to note about the copy is that it will leak if you throw an exception. Since you don't know what the type of T is you have no idea how it will react to being copied. If halfway through the copy it throws an exception you should clean up any memory allocated so far before letting the exception propagate out of the constructor.

You are on the correct track with the assignment operator.

template
LinkedList& LinkedList::operator=(LinkedList byValList)
{
    // BUT this line is not enough
    //     Assignment should make a copy of all the elements.
    std::swap(head, byValList.head);

    // Usually this is implemented as:
    // Now you need to write a version of swap for this class
    // (both member and free standing)
    byValList.swap(*this);

    return *this;
}

I would write them like this:

template
LinkedList::swap(LinkedList& rhs) noexcept // swap is supposed to be 
{                                                // free from exception
    std::swap(head,  rhs.head);                  // throwing
    std::swap(tail,  rhs.tail);
}

template
void swap(LinkedList& lhs, LinkedList& rhs) {lhs.swap(rhs);}

In the destructor:

You don't use curr to do anything useful. Remove it.

Node * curr = head;

        curr = head;

In your insert methods. I personally would return a reference to *this (see below). But in both your insert methods you check for empty is always a bit weird before assigning the other end. I would break the test for empty into its own method empty() then you can test empty() before doing your special case code.

template
LinkedList& LinkedList::insertToTail(T val);

This allows you to use operator chaining.

LinkedList  list;
list.insertToTail(1).insertToTail(2).insertToTail(3);

Nothing wrong with a print method. But I would do three additional things. As the print() method does not modify the content of the list it should be marked as const. Rather than always printing to std::cout I would pass the output stream as a parameter (it can default to std::cout when none is provided. I would also write the output operator operator<< as that is the normal way of printing in C++.

template
void LinkedList::print(std::ostream& stream = std::cout) const;

std::ostream& operator const& data)
{
    data.print(stream);
    return stream;
}

That's an expensive print when done backwards.

But once you have the list reversed. Why not re-use your standard print function?

```
template
void LinkedList::printBackwards(std::ostream& stream = std::cout) const
{
LinkedList rev;
for(Node* curr = rev.head; curr != NULL; curr = curr->next)
{ rev.insertToHead(curr->data);
}
rev.pr

StackExchange Code Review Q#42958, answer score: 31