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 ++;
            }
        }

If you don't want first to be null, then point it to something. You already know how to create a Node (as shown in your else block), so do: first = new Node;. — jamesdlin
Your conditional says if first is null, then use first. This is incorrect. You can't use first if its null. — Simon Whitehead
@BdkFivehunna I rolled back your question to its original form. If you have a different question, ask a new question. Don't replace an existing question. Doing so makes all of the comments and answers look nonsensical. — jamesdlin

Drew Dormann Drew Dormann · Accepted Answer · 2013-04-09T03:21:21

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. :)

Access violation reading location 0xC0000005 C++

3 Answers