HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppModerate

Linked List implementation in C++

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
listimplementationlinked

Problem

I'm learning to implement linked list. I've implemented sorting with merge sort, reversing etc...

LinkedList.cpp

```
#include

template
struct Node
{
T data;
Node * next;
};

template
class LinkedList
{
public:
LinkedList() : head(NULL), size(0) {};
~LinkedList() { destroyList(); };
bool addNode(T data);
bool deleteNode(T data);
Node * searchNode(T data);
void printList();
void reverseList();
void sortList();
private:
Node * head;
int size;
void destroyList();
Node mergeSort(Node head, int total);
Node Merge(Node left, int lcount, Node* right, int rcount);
void print(Node * tmp);
};

template
bool LinkedList::addNode(T data)
{
try
{
Node * tmp = new Node();
tmp->data = data;
tmp->next = head;
head = tmp;
++size;
return true;
}
catch(std::exception & ex)
{
return false;
}
}

template
bool LinkedList::deleteNode(T data)
{
Node curr = head, prev = NULL;

while (curr)
{
if (curr->data == data) break;

prev = curr;
curr = curr->next;
}

if (curr)
{
if (prev)
{
prev->next = curr->next;
}
else
{
head = curr->next;
}
delete(curr);
--size;
return true;
}
else
{
return false;
}
}

template
Node * LinkedList::searchNode(T data)
{
Node * tmp = head;
while (tmp)
{
if (tmp->data == data)
{
return tmp;
}
tmp = tmp->next;
}
return NULL;
}

template
void LinkedList::print(Node * tmp)
{
bool printNewLine = (tmp) ? true : false;
while (tmp)
{
std::cout data next;
}

if (printNewLine)
{
std::cout
void LinkedList::printList()
{
Node * tmp = head;
bool printNewLine = (tmp) ? true : false;
while (tmp)
{
std::cout data next;
}

if (printNewLine)
{
std::cout
void LinkedList::destroyList()
{
Node * tmp = NULL;
while (head)
{
tmp = head;
head = head

Solution

#include 

template 
struct Node
{
  T data;
  Node * next;
};

template 
class LinkedList
{
public:
  LinkedList() : head(NULL), size(0) {};
  ~LinkedList() { destroyList(); };


Why did you make destroyList a seperate method? Why not put it in the destructor?

bool addNode(T data);
  bool deleteNode(T data);
  Node * searchNode(T data);
  void printList();
  void reverseList();
  void sortList();
private:
  Node * head;
  int size;


I recommend some newlines between the data methods and the utility functions.

void destroyList();
  Node* mergeSort(Node * head, int total);
  Node* Merge(Node* left, int lcount, Node* right, int rcount);


I recommend picking a consistent capitalization scheme. This should be merge not Merge.

void print(Node * tmp);
};

template 
bool LinkedList::addNode(T data)
{
try
  {
    Node * tmp = new Node();
    tmp->data = data;
    tmp->next = head;
    head = tmp;
    ++size;
    return true;
  }
catch(std::exception & ex)
  {
    return false;
  }


Don't ever do this. You shouldn't generally catch all exceptions. You should catch only the exception you are interested in. Converting the exception into a return value loses the advantage of an exception. Why did you add this exception logic here?
}

template 
bool LinkedList::deleteNode(T data)
{
  Node *curr = head, *prev = NULL;


I recommend typing whole words not abbreviations.

while (curr)
  {
    if (curr->data == data) break;


Use while(curr && curr->data != data)

prev = curr;
    curr = curr->next;
  }

  if (curr)
    {


You are making use of inconsistent indentation. Pick a scheme and stick to it.

if (prev)
        {
          prev->next = curr->next;
        }
      else
        {
          head = curr->next;
        }
      delete(curr);


Parenthesis not needed

--size;
      return true;
    }
  else
    {
      return false;
    }
}

template 
Node * LinkedList::searchNode(T data)


You are really finding a node rather then searching it.

{
  Node * tmp = head;


tmp should be a banned variable name. all variables are temporary, so pick a better name,

while (tmp)
    {


I suggest

for(Node * tmp = head; tmp; tmp = tmp->next)

I think it'll make the code clearer.

if (tmp->data == data)
        {
          return tmp;
        }
      tmp = tmp->next;
    }
  return NULL;
}

template 
void LinkedList::print(Node * tmp)


Printing is a bit of a odd feature for a linked list class. Typically, you'd except external code to handle that.

{
  bool printNewLine = (tmp) ? true : false;


Use 'bool printNewLine = bool(tmp)';

while (tmp)
    {
      std::cout data next;
    } 

  if (printNewLine)


Rather then doing this. I suggest keeping a copy of the original parameter and testing it here.
{
std::cout next;
}

count = total/2;


Rather then doing this calculation twice, I suggest saving the original version

first = mergeSort(first, count);

  curr = mergeSort(curr, total-count);

  return Merge(first, count, curr, total-count);
}

template 
Node* LinkedList::Merge(Node* left, int lcount, Node* right, int rcount)
{
  Node * h = new Node();
  h->next = NULL;
  Node * tmp = h;


Creating a node during merging seems odd...

while (lcount > 0 && rcount > 0)
    {
      if (left->data data)
        {
          tmp->next = left;
          tmp = tmp->next;


use tmp = left

left = left->next;
          --lcount;
        }
      else if (right->data data)
        {
          tmp->next = right;
          tmp = tmp->next;
          right = right->next;
          --rcount;
        }
      else
        {
          tmp->next = left;
          tmp = tmp->next;
          left = left->next;
          --lcount;

          tmp->next = right;
          tmp = tmp->next;
          right = right->next;
          --rcount;


There is no need for this. Just let one of the above cases handle equal as well.
}
}

while (lcount > 0)
    {
      tmp->next = left;
      tmp = tmp->next;
      left = left->next;
      --lcount;


This pattern is getting common. Think about refactoring this function or writing a function.

}

  while (rcount > 0)
    {
      tmp->next = right;
      tmp = tmp->next;
      right = right->next;
      --rcount;
    }

  tmp = h;
  h = h->next;
  delete(tmp);

  return h;
}


EDIT: My version of merge

```
template
Node LinkedList::Merge(Node left, int count_left, Node* right, int count_right)
{
Node * head = NULL;
Node ** current = &head;

while (count_left > 0 || count_right > 0)
{
if( count_right == 0 || (count_left > 0 && left->data data))
{
*current = left;
left = left->next;
--count_left;
}
else
{
*current = right;
right = right->next;
--count_right;
}
current = &(*curren

Code Snippets

#include <iostream>

template <class T>
struct Node
{
  T data;
  Node * next;
};

template <class T>
class LinkedList
{
public:
  LinkedList() : head(NULL), size(0) {};
  ~LinkedList() { destroyList(); };
bool addNode(T data);
  bool deleteNode(T data);
  Node<T> * searchNode(T data);
  void printList();
  void reverseList();
  void sortList();
private:
  Node<T> * head;
  int size;
void destroyList();
  Node<T>* mergeSort(Node<T> * head, int total);
  Node<T>* Merge(Node<T>* left, int lcount, Node<T>* right, int rcount);
void print(Node<T> * tmp);
};

template <class T>
bool LinkedList<T>::addNode(T data)
{
try
  {
    Node<T> * tmp = new Node<T>();
    tmp->data = data;
    tmp->next = head;
    head = tmp;
    ++size;
    return true;
  }
catch(std::exception & ex)
  {
    return false;
  }
template <class T>
bool LinkedList<T>::deleteNode(T data)
{
  Node<T> *curr = head, *prev = NULL;

Context

StackExchange Code Review Q#4569, answer score: 10

Revisions (0)

No revisions yet.