C++ implementation of a linked list

Can someone please review this code and provide comments?

• Please comment on error handling

-
Is it right to do like this in method LinkedList::InsertFront()?

Node *node = node->CreateNode(key);

```
#include
#include
#include

class LinkedList{
public:
LinkedList(){
head = NULL;
tail = NULL;
};
~LinkedList(){
Node *temp;
while( head!=NULL ) {
temp = head;
head = head->next;
delete temp;
}
}
void InsertFront(int key); /Insert a node in head of the list/
void InsertBack(int key); /Insert a node in end of the list/
int TopBack(); /Get tail node - complexity-O(1) /
int TopFront(); /Get head node - complexity-O(1) /
void PopBack(); /Remove the first element in list.complexity-O(n)/
void PopFront(); /Remove the first element in list.complexity-O(1) /
void FindNode(int key); /Find node O(n)/
void DeleteNode(int key); /Delete first matching node of a given key.complexity-O(n)/
void PrintList();
private:
struct Node {
int item;
Node *next;
Node *CreateNode(int key){
Node *node = new Node();
if(!node)
return NULL;
node->next = NULL;
node->item = key;
}
};
Node *head;
Node *tail;
};

/Insert a node in head of the list/
void
LinkedList::InsertFront(int key) {
Node *node = node->CreateNode(key);
if(!node)
{
std::cout next = head;
} else {
tail = node;
}
head = node;
}

/Insert a node in end of the list/
void
LinkedList::InsertBack(int key) {
Node *node = node->CreateNode(key);
if(!node)
{
std::cout next = node;
}
tail = node;
}

/Get tail node - complexity-O(1) /
int
LinkedList::TopBack(){
if(!tail) {
std::cout item;
}

/Get head node - complexity-O(1) /
int
LinkedList::TopFront(){
if(!head) {
std:

Use C++ Headers

These are C headers.

#include       // Also some white space to make it readable
#include      // would be nice.

The C++ equivalent are:

#include 
#include 

The difference is that the C++ versions put the function into the stanadrd namespace std so that we avoid name collisions.

You have not implemented the rule of three.

LinkedList   x;
x. InsertFront(1);
LinkedList   y(x); // This is allowed because the compiler
                   // automatically generates a copy constructor
                   // and a copy assignment operator.

You will notice the above compiles (as the compiler generated the copy constructor). But because the compiler generated version does a shallow copy both x and y point at the same list.

x.head -> 1
x.tail ---^
y.head ---^
y.tail ---^

The trouble is that at the end of scope. Both objects get destroyed. 'y' will get destroyed first calling its destructor and calling delete on the node. Then 'x' will be destroyed and it will also call delete on its one node (but this node has already been deleted so this is illegal).

Now the compiler generated methods are usually exactly what you want. But when your class contains an "owned" pointer then you need to do some extra work as the default implementation simply does a shallow copy.

Rule of 5 (Optional)

In C++11 they introduced move semantics to the language. So it has become pretty standard to implement move semantics for container class (as it makes a lot of things very effecient).

Use Initializer List

class LinkedList{
public:
   LinkedList(){
      head = NULL;
      tail = NULL;
   };  // This ';' is not required.

Use the initializer list to initialize member variables in the constructor.

LinkedList()
      : head(nullptr) // In C++11 we introduced nullptr to replace NULL
      , tail(nullptr) // This is because of the type information.
   {}

Return by reference (maybe)

int TopBack();
   int TopFront();

This code works fine. But if you return by reference you can alter the value while it is in the list. This can help if you don't want to pop the value and then replace it.

I would have done:

int& TopBack();
   int& TopFront();

Then in my code I can do:

LinkedList x;
   // Add stuff to x

   x.TopFront() = 5; // Change the top front value to 5.

What does find do?

void FindNode(int key);          /*Find node O(n)*/

It does not return anything?

Printing

void PrintList();

Sure you can print the list. But print it to where? Why not pass the stream you want to print on as a parameter. You can default the stream to std::cout.

void PrintList(std::ostream& out = std::cout);

Create should just be a constructor.

Sure you can have a createNode() function. But to use this it should be a static method otherwise you need a member to call it.

Node *CreateNode(int key){

But it's easy to call the constructor directly.

Node* newNode = Node::CreateNode(5);
      // or
      Node* newNode = new Node(5);

new will not fail

The new function will never return nullptr. If it fails it will throw an exception. As a result there is no need (or point) in checking the return value of new.

Node *node = new Node();
         if(!node)                 // This will never fire.
            return NULL;

Here there is no point in checking for nullptr here.

Node *node = node->CreateNode(key);
   if(!node)

StackExchange Code Review Q#146357, answer score: 4