Circular doubly linked list with templates

I tried to write some code namely "Circular doubly linked list" and I would like to use templates to do this.

This is the first time I use templates and throws some exceptions.
I would like to know if sth is badly writen, sth must be changed or I need to rewrite everything.

Last but not least, what is the best framework to unit test in C++ (and mostly without too much effort to implement in IDE, I use CLion) like jUnit in Java?

ListNode class

#ifndef CYCLIC_LIST_LISTNODE_H
#define CYCLIC_LIST_LISTNODE_H

template
class List;

template
class ListNode{
private:
    DT dataItem;
    ListNode *prior, *next;

    ListNode(const DT & data, ListNode *prior = nullptr, ListNode *next = nullptr)
            : dataItem(data), prior(prior), next(next) {}

    ListNode() : dataItem(), prior(nullptr), next(nullptr) {}

    friend class List;
};

#endif //CYCLIC_LIST_LISTNODE_H

List class

```
#ifndef CYCLIC_LIST_LIST_H
#define CYCLIC_LIST_LIST_H

#include "ListNode.h"
#include
#include
#include

template
class ListNode;

template
class List{
private:
ListNode *head;
ListNode *cursor;
public:
List() : head(nullptr), cursor(nullptr) {}
~List();

void insert(const DT &newDataItem) throw(std::bad_alloc);
void remove() throw(std::logic_error);
void replace(const DT & newDataItem) throw(std::logic_error);
void clear();

bool isEmpty() const;

void gotoBeginning() throw(std::logic_error);
void gotoEnd() throw(std::logic_error);
void gotoNext() throw(std::logic_error);
void gotoPrior() throw(std::logic_error);

DT getCursor() const throw(std::logic_error);

void showStructure() const;
};

template
void List::replace(const DT & newDataItem) throw(std::logic_error){
if(cursor == nullptr || isEmpty())
throw std::logic_error("Empty list!");

cursor->dataItem = newDataItem;
}

template
void List::clear() {
if(!isEmpty())
{
ListNode* cursor2;
cursor2 = head->prior;
c

Review

ListNode:

Are you sure you want this constructor?

ListNode() : dataItem(), prior(nullptr), next(nullptr) {}

The problem I see is that it default constructs the data object. Not all data objects can be default constructed. The other question I would ask is: Can you create a node with no item in it?

In C++11 we introduced move semantics so I would expect to see a move constructor on this class.

In C++11 we introduced perfect forwarding. So though not required it is sometimes nice to build an item (of type DT) in place with the constructors parameters. You can achieve this using perfect forwarding. If you look at std::vector you can see this in practive in the emplace_back() method.

List

You don't need to forward declare a class if you have already included the header file:

#include "ListNode.h"

// This is not needed.    
template
class ListNode;

Design

The cursor concept. I see your logic.

ListNode *cursor;
void gotoBeginning() throw(std::logic_error);
void gotoEnd() throw(std::logic_error);
void gotoNext() throw(std::logic_error);
void gotoPrior() throw(std::logic_error);

DT getCursor() const throw(std::logic_error);

But this means all code is using the same cursor. This makes threaded code impossible and even single threaded code hard if you are using the list in more than one place.

In C++ we end to use the concept of an iterator. The iterator is an object itself that tracks the current position in the list. Can be copied so you can save a position; incremented and decremented to move along the container; de-referenced to get the element value. It is then used in conjunction with the methods of the container for insertion/deletion. But the important point; it is a separate object.

Exception Specifications

Most languages have decided that exception specifications are a bad idea. In Java they devolve (over time) to throw Exception and in C++ the only useful one we found was the specification that said this function does not throw.

void insert(const DT &newDataItem) throw(std::bad_alloc);
void remove() throw(std::logic_error);
void replace(const DT & newDataItem) throw(std::logic_error);
void gotoBeginning() throw(std::logic_error);
void gotoEnd() throw(std::logic_error);
void gotoNext() throw(std::logic_error);
void gotoPrior() throw(std::logic_error);
DT getCursor() const throw(std::logic_error);

As a result. C++11 deprecated the throw specifier on functions/methods and introduced the noexcept specifier. So either your function is exception safe and throws nothing or it can potentially throw.

Sentinel

Your code uses a nullptr head to specify an empty list. The problem is that your code basically has two versions of every function. Code to handle the empty list situation and code to handle the non empty list.

There is a technique that uses a sentinel object. This is a fake ListNode object (with no data). That is always in the list (the sentinel). This simplifies the code tremendously as you no longer need to check for the empty list when removing or adding elements.

Move Semantics

Your list does not support move semantics or emplace semantics. This can make the code very inefficient.

// COPY (current implementation)
template
void List::replace(const DT & newDataItem) throw(std::logic_error)
{
    if(cursor == nullptr || isEmpty())
        throw std::logic_error("Empty list!");

    cursor->dataItem = newDataItem;
}

// Move
template
void List::replace(const DT&& newDataItem) throw(std::logic_error)
{
    if(cursor == nullptr || isEmpty())
        throw std::logic_error("Empty list!");

    // Uses the objects move assignment operator
    // to move the object over the current object.
    // For large objects this can be significantly more efficient).
    cursor->dataItem = std::move(newDataItem);
}

Bug

template
void List::clear() {
    if(!isEmpty())
    {
        ListNode* cursor2;
        cursor2 = head->prior;   // I believe this is a bug.
                                 // As a result you will probably only
                                 // remove one item from the list.

Bug

template
void List::remove() throw(std::logic_error) {
    // STUFF
    delete cursor;

    cursor = temp;  This is a bug if head is nullptr.
}

Biggest Bug

You don't implement the rule of three/Five.

You class manages resources (hence the destructor). But you have not defined the copy/move semantics of the class. As a result the compiler has provided default implementations of each (which don't work when you are managing resources). Hence the rule.

```
{
List l1;
l1.insert(5);
List l2(l1); // Makes a copy of l1
}
// Both objects are destroyed.
// But because you don't not implement copy semantics the
// compiler generated version is used which does a shallow copy
// and thus both objects have the same pointer. This may not seem
// like a problem but both destructors are going to use that pointer
// to head t

StackExchange Code Review Q#150744, answer score: 3