Copy constructor and assignment operator for doubly linked list

I have implemented a doubly linked list in C++. I have tested it and everything works as expected, but I am not sure if my copy constructor and assignment operator perform a deep copy.

• Can someone tell whether I am doing a shallow or deep copy?

• Just by looking at my code, does my destructor seem to work properly?

• I think I am leaking memory. Can anybody confirm? I will run valgrind as soon as I get a chance.

```
#include

template
class list {

public:

list(); // constructor
list(const list &l); // copy constructor
list &operator=(const list &l); // assignment operator
~list(); // destructor

// Returns number of elements in the list
unsigned size();

// Returns true if the list is empty, false otherwise.
bool isEmpty() const;

// Inserts element to front of list
void insertFront(const T &val);

// Inserts element to the end of list
void insertBack(const T &val);

// Returns the values of the front element in the list
T front();

// Returns the value of the back element of the list.
T back();

// Deletes the front element of the list and returns its value
void removeFront();

// Deletes the back element of the list and returns its value
void removeBack();

// Prints each element of the list in order
void printForward();

// Prints each element of the list in reverse order
void printReverse();

private:
struct node {
node *next;
node *prev;
T value;
};

node *first; // The pointer to the first node
node *last; // The pointer to the last node
unsigned length; // holds number of elements in the list

// Initializes empty list
void createEmpty();

// Removes all of the elements in the list
void removeAll();

// Makes copy of all of the elements in the list
void copyAll(const list &l);

};

There's a few stylistic or design things that I noticed, but please pay special attention to the first two (especially the first item). The first two are not style or design related but rather actual bugs that are very likely to happen (well, the second one not so much).

NOTE: This item is a major bug. It's a "this code could never actually be used" level bug.

Why are you placement-new'ing on top of the default-constructed element of node? This is unnecessary unless you're trying to get around some rather odd aversion to a copy constructor or assignment operator.

If you're going to do it this way (and I highly suggest that you don't), you need to manually destruct the element ((newNode->value).~T()) before you placement new over it.

To illustrate the problem, imagine that your're containing a type that sets up some kind of resources in its constructor. As the destructor never gets called for the initial member of node (the one that gets created when node is created), those resources will leak.

I would just pass the object to node through a constructor and let the compiler work its magic before trying to unnecessarily placement-new things:

struct node {
    T value;
    node* next;
    node* prev;
node(const T& val, node* const next = nullptr, node* const prev = nullptr)
    : value(val), next(next), prev(prev)
{ }
};

When you're dealing with memory details, you must be very conscious of exceptions.

In particular, if your new statements throw exceptions after your length++ statements, the length is then incorrect. Luckily the solution is simple: just allocate the memory before incrementing length.

Your indenting is inconsistent (4 spaces sometimes, and 2 spaces others)

The void returns in removeFront and removeBack that do not affect control flow are a bit odd.

front and back could (and probably should) return a reference instead of a copy (as all of the standard library containers do). Note that you would then want two versions: a const one that returns a const reference, and a non-const one that returns a mutable reference.

You've implemented more of a deque than a list. How do you iterate over the list?

I suggest going the style of the standard library and making iterators.

As this is just an academic exercise (if not, use std::list), there's no need to put in all of the effort, but if you wanted, you could even do standard style iterators: this and this should prove helpful if you go this route.

This will allow you to iterate over the list, and, provided you write a iterator insert(iterator, const T&) method, your insertFront and insertBack methods become trivial (insert(end(), value) for example).

Your printing methods don't particularly belong in the class.

What if instead of printing a message when the list is empty, I want to do nothing? What if instead of a space separator, I want a newline? There's so many decisions that you've hard coded that it makes the method useless to a consumer of the class. Let consumers of the class handle output formatting.

(Fun bonus with iterators: just std::copy into a std::ostream_iterator and you have your print methods already written.)

createEmpty() shouldn't exist. That should be part of initialization lists.

StackExchange Code Review Q#26451, answer score: 5