Skip to main content
8 of 9
added 22 characters in body

Take care to be explicit about compiler generated functions

Effectively your List class violates the Rule of Three/Five/Zero, thus the compiler generated copy/move constructors will create unexpected side effects by implementing a shallow copy mechanism.

Let's suppose you have code like

int main() {
    List l1;
    List l2;
    
    l1.push_back(5);
    l1.push_back(42);
    l1.push_back(21);

    l2 = l1; // Copy the list
    *l1.begin() = 80; // Change the original list
    
    for(auto value : l2) {
        std::cout << value << std::endl;
    }
}

The output is

80
42
21

I doubt you really intended that. See the Live Demo please.

Why not making your class a template?

using T = int;

looks strange.

Why not making the List class a template like

template<typename T>
class List {
    // ...
};

Compiles and works the same way here

Prefer smart pointers instead of raw pointers

You should prefer using smart pointers instead of managing new and delete yourself.

I have evolved the demo from above with changing the relevant parts

class Node;
class Node {
public:
    T data;
    std::shared_ptr<Node> next;
    std::shared_ptr<Node> prev;

public:
    explicit Node(std::shared_ptr<Node> next_, std::shared_ptr<Node> prev_) : next(next_), prev(prev_) {}

    explicit Node(const T &_data, std::shared_ptr<Node> next_, std::shared_ptr<Node> prev_) :
            data(_data), next(next_), prev(prev_) {}

    ~Node() {}
};

class Iterator : public std::iterator<std::bidirectional_iterator_tag, T> {
public:
    std::weak_ptr<Node> position;
    std::weak_ptr<Node> tail;

public:
    Iterator(std::shared_ptr<Node> node) : position(node) {}

    Iterator(std::shared_ptr<Node>node, std::shared_ptr<Node>_tail) : position(node), tail(_tail) {
    }

    Iterator &operator++() noexcept {
        position = position.lock()->next;
        return *this;
    }

    Iterator operator++(int) noexcept {
        auto old = *this;
        position = position->next;
        return old;
    }

    Iterator &operator--() noexcept {
        if (position == nullptr) position = tail;
        else
            position = position->prev;
        return *this;
    }

    Iterator operator--(int) noexcept {
        auto old = *this;
        if (position == nullptr) position = tail;
        else
            position = position->prev;
        return old;
    }

    T &operator*() const noexcept {
        return (position.lock()->data);
    }

    bool operator!=(Iterator other) noexcept {
        return position.lock().get() != other.position.lock().get();
    }

    bool operator==(Iterator other) noexcept {
        return position.lock().get() == other.position.lock().get();
    }


    ~Iterator() {}
};

Solve the shallow copy problem

As mentioned in my 1st point you have a problem with The Rule of Three/Five. Your List class will be shallow copied. So you should provide the relevant operations.
This will be a lot easier using smart pointers as mentioned in my previous point

List(const List<T>& rhs) {
    copy(rhs);
}

List(const List<T>&& rhs) {
    move(std::move(rhs));
}

List& operator=(const List<T>& rhs) {
    copy(rhs);
    return *this;
}

List& operator=(const List<T>&& rhs) {
    move(std::move(rhs));
    return *this;
}


void copy(const List<T>& rhs) {
    clear();
    for(auto value : rhs) {
        push_back(value);
    }
}

void move(const List<T>&& rhs) {
    head = std::move(rhs.head);
    tail = std::move(rhs.tail);
}

void clear() {
    std::shared_ptr<Node> temp = head;
    std::shared_ptr<Node> next;
    do {
        if(temp) {
            next = temp->next;
        }
        temp = nullptr;
    } while(next);
}

~List() {
}

Output is as expected for the example mentioned in the 1st paragraph:

5
42
21

Again you can check the improved code here.