Access violation reading location 0xC0000005 C++

Question

My add function clearly has a problem as it is dereferencing first and first is pointing to nothing because of that. I just don't know how to fix it so that it isn't a null pointer.

    struct Node
    {
        int data;
        Node *next;
    };

    class LinkedList
    {
        Node *first;
        Node *last;
        int count;
        public:

        LinkedList()
        {
            first = NULL;
            last = NULL;
            count = 0;
        }


        void Add(int item)
        {
            if (first == NULL)
            {
                first->data = item;
                last->data = item;
                last->next = NULL;
                first->next = last;
                count = 1;
            }
            else
            {
                Node *newNode = new Node;
                newNode->data = last->data;
                newNode->next = last;
                last->data = item;
                last->next = NULL;
                count ++;
            }
        }

Answer 1

You have a lot of code in common between the if and the else.

        if (first == NULL)
        {
            first->data = item;
            last->data = item;
            last->next = NULL;
            first->next = last;
            count = 1;
        }
        else
        {
            Node *newNode = new Node;
            newNode->data = last->data;
            newNode->next = last;
            last->data = item;
            last->next = NULL;
            count ++;
        }

In the if, you increment count from 0 to 1. In the else, you also increment it.

count is always getting incremented. So you don't need to type it twice.

        if (first == NULL)
        {
            first->data = item;
            last->data = item;
            last->next = NULL;
            first->next = last;
        }
        else
        {
            Node *newNode = new Node;
            newNode->data = last->data;
            newNode->next = last;
            last->data = item;
            last->next = NULL;
        }
        count ++;

You're also setting last->data to item in both of them.

And you're setting last->next to NULL in both of them.

        if (first == NULL)
        {
            first->data = item;
            first->next = last;
        }
        else
        {
            Node *newNode = new Node;
            newNode->data = last->data;
            newNode->next = last;
        }
        last->data = item;
        last->next = NULL;
        count ++;

You also forgot to create a new Node when it's the first new node.

        if (first == NULL)
        {
            Node *newNode = new Node;   // Added
            first = newNode;            // Added
            last = newNode;             // Added
            first->data = item;
            first->next = last;
        }
        else
        {
            Node *newNode = new Node;
            newNode->data = last->data;
            newNode->next = last;
        }
        last->data = item;
        last->next = NULL;
        count ++;

The first->data = item in your if is redundant. first is the same as last there, and last->data = item is already happening.

        if (first == NULL)
        {
            Node *newNode = new Node;
            first = newNode; 
            last = newNode;
            // Removed
            first->next = last;
        }
        else
        {
            Node *newNode = new Node;
            newNode->data = last->data;
            newNode->next = last;
        }
        last->data = item;
        last->next = NULL;
        count ++;

And since first and newNode have the same value in that if, we can use the variable names interchangeably.

        if (first == NULL)
        {
            Node *newNode = new Node; 
            first = newNode;            // These two pointers are equal!
            last = newNode;
            newNode->next = last;       // (same pointer)
        }
        else
        {
            Node *newNode = new Node;
            newNode->data = last->data;
            newNode->next = last;
        }
        last->data = item;
        last->next = NULL;
        count ++;

Now, almost everything in your else is also in your if. It can all be moved out.

        Node *newNode = new Node; 
        if (first == NULL)
        {
            first = newNode;
            last = newNode;
        }
        else
        {
            newNode->data = last->data;
        }
        newNode->next = last;
        last->data = item;
        last->next = NULL;
        count ++;

That code should be more understandable now, too. The lesson: Don't Repeat Yourself. :)

Answer 2

Have a look at linked list

There are few details, to start with when first == NULL you need to create first, insert it into linked list and hook it up, see linked article for some algorithms.

I would say the simplest is single linked list with a header node (instead of first *) which could point to itself, but there are many ways to implement linked list and it depends on what you choose how to hook up the elements.

It depends on what you are after but if you just need something working then you could pick up from boost intrusive circular slist algorithms where you just define your own struct with data and next pointer, tell it how to access next and use provided algorithms to do all the work (link and unlink nodes).

Answer 3

if (first == NULL)
{
    /* if first is NULL dereference it. Hooray! */
    first->data = item;
    ...