How do I fix this exception being thrown at a certain part of my code?

Question

When I build the code, I don't get any errors in the output window. However, after running it, the compiler throws an exception (I'll comment where it is being thrown) at my code saying "Exception thrown: read access violation. temp was 0xCDCDCDCD.".

I tried researching what this error is, and I found that this is for unassigned memory, but I don't see where something is being unassigned.

This is my Linked List .cpp file. The exception is thrown at a line towards the end of this file.

#include "linkedlist.h"

struct ll::node 
{
    weapons data;
    node* next;
};

ll::ll()
{
    head = NULL;
}

ll::~ll()
{
    while (head != NULL)
    {
        node* temp = head;
        head = head->next;
        delete temp;
    }
}

void ll::addItem(weapons obj)
{
    node* newNode = new node;
    node* temp = head;

    newNode->data = obj;

    if (head == NULL)
        head = newNode;
    return;

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

void ll::displayItems()
{
    for (node* temp = head; temp != NULL; temp = temp->next)
    {
            temp->data.getDescription(); //EXCEPTION THROWN HERE
    }
}

This file has the inherited class "Weapons" which is the object that is being called as "temp->data". As well as where I have "getDescription".

#include <vector>

using namespace std;

//base class
class inventory
{
protected:
    //number of items in inventory
    int mNumItems;
public:
    //getters
    void displayInv();
    int getNumItems();
    virtual void getDescription();
};

//weapon class
class weapons : public inventory
{
private:
    //name of object
    string mName;
    //what the object is
    string mInfo;
    //how much of the object
    int mAmount;
    //how much damage does it do
    double mDamage;

public:
    //constructor
    weapons();
    weapons(string, string, double, int);
    //getters
    string getName();
    void getDescription();
    int getAmount();
    double getDamage();
    string getInfo();

    //mutators
    void setAmount(int);
};

This is where I define weapons

//weapon class
weapons::weapons()
{
    mName = " ";
    mInfo = " ";
    mDamage = 0.0;
    mAmount = 0;
}

weapons::weapons(string name, string info, double dmg, int amt)
{
    mName = name;
    mInfo = info;
    mDamage = dmg;
    mAmount = amt;
}

string weapons::getName()
{
    return mName;
}

int weapons::getAmount()
{
    return mAmount;
}

double weapons::getDamage()
{
    return mDamage;
}

string weapons::getInfo()
{
    return mInfo;
}

void weapons::getDescription()
{
    cout << getName() << ", " << getDamage() << " damage, " << getInfo() << " Amount: " << getAmount() << endl;
}

void weapons::setAmount(int amt)
{
    mAmount = amt;
}

Let me know if I need to include anymore files!

I get the expected results, which is for it to describe an item which I have in the Linked List. Unfortunately, my only problem is that this exception is being thrown.

Answer 1

Problem

In

struct ll::node 
{
    weapons data;
    node* next;
};

and

void ll::addItem(weapons obj)
{
    node* newNode = new node; // leaks if node not added
    node* temp = head;

    newNode->data = obj;

    if (head == NULL)
        head = newNode;
    return; // this is a NASTY bug of a different sort. Most of the time 
            // the function will exit without doing ANYTHING

    while (temp->next != NULL)
    {
        temp = temp->next;
    }
    if (temp->next == NULL) // the only way out of the above loop is if 
                            // temp->next == NULL. This if is redundant.
    {
        temp->next = newNode;
        return;
    }   
}

Nothing ever sets newNode->next to a safe value. That allows

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

to fail because there are no guarantees that temp->next is ever NULL and the loop goes marching off the end of the list.

Solution

Force next to a safe value.

struct ll::node 
{
    weapons data;
    node* next = NULL;
};

Or a more versatile version

struct ll::node 
{
    weapons data;
    node* next;
    node(const weapons & weap, // const reference eliminates a possible copy
         node * link = NULL): // if you know what the next link will be, 
                              // you can add it here. If not, it's always NULL
        data(weap),
        next(link) 
    {
    }    
};

addItem now looks something like

void ll::addItem(const weapons & obj) 
{
    if (head == NULL)
    {
        head = new node(obj); // only making node if we need it
                              // less chance of leak
    }
    else
    {
        node * temp = head;
        while (temp->next != NULL)
        {
            temp = temp->next;
        }
        temp->next = newNode(obj);
    }
}

But you can do something really sneaky here to make life easier. head is really a next pointer by another name, so if you can abstract the different name... And we can by tracking a pointer to next rather than a pointer to the node. This is really handy when you have to insert or remove an item: You have a reference both to the node in question and the insertion point in the previous node.

void ll::addItem(const weapons & obj) 
{
    node ** temp = &head; // get pointer to insertion point
    while ((*temp) != NULL) // next node, including head, is not null
    {
        temp = &(*temp)->next; // get pointer to next insertion point
    }
    *temp = newNode(obj); // insert node
}

Half the code. Example of how this helps remove:

void ll::removeItem(const weapons & obj) 
{
    node ** temp = &head; 
    while ((*temp) != NULL && (*temp)->data != obj) 
    {
        temp = &(*temp)->next; 
    }
    if  (*temp != NULL) // found it!
    {
        node * rem = *temp; // get node to remove so we don't lose it when we relink
        *temp = rem->next; // point at item after rem
        delete rem; // release item
    }
}