Linked List design and implementation - C++

Seeking recommendations on how to improve this code, LL always seem to be a struggle.

A basic linked list that does include next and previous pointers. I used a struct inside of a class, which I'm not sure if that's frowned upon. The program includes 5 basic member functions, one to add to the head of the list, one to add to the tail of the list, one to delete the head, one to delete any index, one that checks the list for duplicate data (based on the data variable in the struct), and a print. Everything works as it should, but I'm just not sure if I was efficient, in fact I'm pretty sure I wasn't. Sorry If im forgetting something, but I'm new here!

1.

class list
{
private:
    struct node
    {
        int data;
        node* next;
        node* prev;
    };

    typedef node* nodePtr;

    nodePtr head;
    nodePtr temp;
    nodePtr prev;

public:
    list(); 
    void add_to_head(int);
    void add_to_tail(int);
    void delete_index(int);
    void check_duplicates(int);
    void print();
};

2.

```
#include
using std:: cout;
using std:: endl;

list::list(void)
{
head = NULL;
temp = NULL;
prev = NULL;
}

void list:: add_to_head(int num)
{
nodePtr n = new node;

n->data = num;

if(head == NULL)
{
n->prev = NULL;
n->next = NULL;
head = n;

}
else
{
n->next = head;
n->prev = NULL;
head = n;
}

return;
}

void list::add_to_tail(int num)
{
nodePtr n = new node;

n->data = num;

if(head == NULL)
{
n->prev = NULL;
n->next = NULL;
head = n;
}
else
{
temp = head;

while(temp->next != NULL)
{
temp = temp->next;
}

temp->next = n;
n->prev = temp;
n->next = NULL;
}

return;
}

void list::delete_index(int index)
{
if(index == 0)
{
temp = head->next;
temp ->prev = NULL;

prev = head;
delete prev;

You can simplify your code a lot.

if(head == NULL)
    {
        n->prev = NULL;
        n->next = NULL;
        head = n;
    }
    else
    {
        n->next = head;
        n->prev = NULL;
        head = n;
    }

Both sides of this else look identical.

I think you can shorten your code with:

void list:: add_to_head(int num)
{
    nodePtr oldHead = head;
    head = new node{num, oldHead, nullptr);
    // Also you forget to do this bit.
    if (oldHead) {
        oldHead->prev = head;
    }
}

@pm100 covered the temporary variable.

nodePtr temp;
nodePtr prev;

You don't need these as a class member. It should be local to the member functions where they are used.

If I was you I would add another member though.

nodePtr tail;

Then you don't need to search for the tail every time you add a new element.

Const correctness. In a lot of code we pass variables by reference and const reference. If you pass an object by const reference you can only call const members on it.

So you should take care to mark functions that don't mutate the object as const. This will allow you to call the methods from a const reference. Also it enables some compiler optimizations when it knows you are not mutating the object.

So the following functions should probably be const.

void check_duplicates(int) const;
void print() const;

Your print() is fine. But I would make it more generic. Rather than print to std::cout you should print to a generic stream (you can always default to std:::cout.

print(std::ostream& str = std::cout) const;

Also in C++ printing is usually done via operator<<. So I would add this function. All it needs to do is call the print function passing the stream.

std::ostream& operator<<(std::ostream& str, List const& data)
{
     data.print(str);
     return str;
}

StackExchange Code Review Q#134874, answer score: 3