Skip to main content
replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

Its short for a reason. Adding std:: as a prefix is not that much of a burden. Get used to it. Also worth a read Why is “using namespace std” considered bad practice?Why is “using namespace std” considered bad practice?. It will explain in detail why it is bad practice.

Its short for a reason. Adding std:: as a prefix is not that much of a burden. Get used to it. Also worth a read Why is “using namespace std” considered bad practice?. It will explain in detail why it is bad practice.

Its short for a reason. Adding std:: as a prefix is not that much of a burden. Get used to it. Also worth a read Why is “using namespace std” considered bad practice?. It will explain in detail why it is bad practice.

added 1 character in body
Source Link
Loki Astari
  • 97.7k
  • 5
  • 126
  • 341

I always do most specific to least. So your local header files first. Then C++ header files. Then C header files. Also add a space before "<memory.h"<memory.h>.

I always do most specific to least. So your local header files first. Then C++ header files. Then C header files. Also add a space before "<memory.h".

I always do most specific to least. So your local header files first. Then C++ header files. Then C header files. Also add a space before <memory.h>.

Source Link
Loki Astari
  • 97.7k
  • 5
  • 126
  • 341

###LinkedList Header

Don't use identifiers with a leading underscore:

#ifndef _NEW_LL_H
#define _NEW_LL_H

These two are not valid (reserved for the implementation).

Header inclusion order:

#include<memory>
#include "node.h"

I always do most specific to least. So your local header files first. Then C++ header files. Then C header files. Also add a space before "<memory.h".

Put the & with the type (its part of the type information).

  LinkedList(LinkedList const & ll);  // copy constructor
  LinkedList& operator=(LinkedList const & ll); // copy assignment
  LinkedList(LinkedList && ll); // move constructor
  LinkedList& operator=(LinkedList && ll);  // move assignment

You have a normal append.

  void append(T const& item);

But your list is move aware. So it seems like you should be able to move elements into the list.

  void append(T&& item);
  template<typename... Args>
  void append(Args&&... args);  // append item but use its constructor.

###Node Header

Don't use identifiers with a leading underscore:

#ifndef _NODE_H
#define _NODE_H

These two are not valid (reserved for the implementation).

You have normal constructors:

Node(T const& t);  // default constructor
Node(Node const& insert); // copy constructor

But what about the move constructors.

Node(Node&& move);

###Source File review:

Don't do this:

using namespace std;

Its short for a reason. Adding std:: as a prefix is not that much of a burden. Get used to it. Also worth a read Why is “using namespace std” considered bad practice?. It will explain in detail why it is bad practice.

This does not work if the source is empty.

template <typename T>
LinkedList<T>& LinkedList<T>::operator=(LinkedList<T> const & ll) {
  if(ll.root) root = make_unique<Node>(*ll.root);
} // copy assignment

This means if you try and copy a list (that happens to be empty) nothing happens. Which is not what you want. If the source is empty then you want your list to become empty (not keep its current content). just convert to using the copy and swap idiom.

Prefer to use the initializer list than the body.

template <typename T>
LinkedList<T>::LinkedList(LinkedList<T> && ll) {
  root = move(ll.root);
} // move constructor

Here you are basically initializing root with nullptr then immediately assigning over it.

You can simplify this a lot. It should not matter if the root is nullptr or not. You are always adding to a thing that is unique_ptr<Node>. Just find the correct one then call make_unique()

template <typename T>
void LinkedList<T>::append(T const& item) {
  if(root==nullptr) {
    root = make_unique<Node<T>>(item);
    return;
  }
  Node<T> *tmpNode = root.get();
  while(tmpNode->next!=nullptr) tmpNode=tmpNode->next.get();
  tmpNode->next = make_unique<Node<T>>(item);
}

Your delete is basically a search() followed by a delete. Why not reduce redundant code by moving the search into its own private doSearch() function that returns a Node*. Then the public functions can use this private function and return the appropriate value.

template <typename T>
bool LinkedList<T>::delete_node(T const& item) {
  if(root->item == item) {
    root = move(root->next);
    return true;
  }
  Node<T> *tmpNode = root.get();
  while(tmpNode->next!=nullptr) {
    if(tmpNode->next->item == item) {
      tmpNode->next = move(tmpNode->next->next);
      return true;
    }
    tmpNode = tmpNode->next.get();
  }
  return false;
}

If you are going to print. Then a least allow the user to specify what stream you want to print too (you can provide a default version):

template <typename T>
void LinkedList<T>::print(std::ostream& out = std::cout)
{
}

Note the default way to print something in C++ is to use the operator<< so you may as well define one of those while you are at it:

frined std::ostream& operator<<(std::ostream& str, LinkedList const& list)
{
    list.print(str);
    return str;
}