_BLOCK_TYPE_IS_VALID(pHead->nBlockUse) after "return 0"

Question

I'm writing a program where I have to write my own string class and a potion class. I know that this run-time error is usually caused when the program is deleting something that wasn't allocated, but it occurs after the 'return 0' in main. I've debugged it line-by-line and tried some things to try and eliminate the run-time error, but nothing has really worked. Is there anybody that can help me out with this?

Here is my code:

//Main
#include "Potion.h"
#include <iostream>
using std::cout;
using std::endl;

void TotalCost( Potion ArrayofPotions[5] )
{
    String Currency[4] = {"Platinum", "Gold", "Silver" ,"Copper"};
    int TotalCost[4] = {0, 0, 0, 0};
    int Cost[4] = {0, 0, 0, 0};

    for (short i = 0; i < 5; i++)
    {
        for (short k = 0; k < 4; k++)
        {
            Cost[k] = ArrayofPotions[i].ConvertCost(k);
            TotalCost[k] += Cost[k];

            if ( i != 0 )
                TotalCost[k] = ArrayofPotions[k].CalculateCost(TotalCost[k - 1], TotalCost[k]);
        }
    }
    cout << "\nTotal cost for all potions: ";

    for (short i = 0; i < 4; i++)
    {
        cout << TotalCost[i] << ' ';
        Currency[i].Display();
    }
}

int main()
{
    Potion Haggling("Potion of Haggling", "You haggle for 10% better prices for 30 seconds",
                "Weak", "0.80.0.4.");

    Potion Strength("Draught of Strength", "You can carry 20% more for 300 seconds", 
                "Low", "2.60.5.1.");

    Potion Health("Solution of Health", "You are 30% tougher for 60 seconds",
              "Mild", "2.20.5.1.");

    Potion Stealth("Philter of Stealth", "You are 40% harder to detect for 60 seconds",
               "Moderate", "0.90.5.1.");

    Potion Waterbreathing("Elixir of Waterbreathing", "You can breathe underwater for 60 seconds",
                      "Strong", "2.10.5.0.");

    Potion ArrayOfPotions[5] = {Haggling, Strength, Health, Stealth, Waterbreathing};

    for (short i = 0; i < 5; i++)
        ArrayOfPotions[i].DisplayPotions();

    TotalCost(ArrayOfPotions);

    system("pause");
    return 0;
}

//String class
class String
{
public:
    String() : m_str(nullptr)
    { }
    String(char chr) : m_str(nullptr)
    {
        m_str = new char;
        *m_str = ch;
    }
    String(char * str)
    {
        if (str != nullptr)
        {
            m_str = new char[strlen(str) + 1];
            strcpy(m_str, str);
        }
    }
    String(const String & copy) : m_str(copy.m_str)
    { }
    String& operator=(const String & rhs)
    {
        if ( this != &rhs )
        {
            delete [] m_str;
            m_str = new char[strlen(rhs.m_str) + 1];
            strcpy(m_str, rhs.m_str);
        }

        return *this;
    }
    ~String()
    {
        delete [] m_str;
    }
    void Display() const
    {
        cout << m_str;
    }
    char * GetStr() const
    {
        return m_str;
    }
    String Upper()
    {
        char * check = this->m_str;

        for (unsigned short i = 0; i < strlen( this->m_str ); i++, check++)
        {
            if ( *check > 96 && *check < 123 )
            {
                *check -= 32;
            }
            else
                m_str = this->m_str;
        }
        return m_str;
    }
private:
    char * m_str;
};

//Potion class

#include "String.h"

class Potion
{
public:    
    Potion() : m_name(nullptr), m_description(nullptr), m_potency(nullptr),
               m_cost(nullptr)
    { }
    Potion(String name, String description, String potency, String cost)
    {
        m_name = name;
        m_description = description;
        m_potency = potency.Upper();
        m_cost = cost;
    }
    Potion(const Potion & copy) : m_name(copy.m_name), m_description(copy.m_description),
                                  m_potency(copy.m_potency), m_cost(copy.m_cost)
    { }
    int ConvertCost(int index)
    {
        int cost = 0;
        char * new_cost = m_cost.GetStr();
        char * temp_string = new char[strlen( new_cost ) + 1];

        strcpy(temp_string, new_cost);

        char * temp = strtok( temp_string, "." );

        for (short k = 0; k != index; k++)
            temp = strtok( NULL, "." );

        cost = atoi( temp );

        delete [] temp_string;

        return cost;
    }
    int CalculateCost(int & cost1, int cost2)
    {
        if (cost > 99)
        {
            cost1++;
            cost2 -= 100;
        }
        return cost2;
    }
    void DisplayPotions()
    {
        String Currency[4] = {"Platinum", "Gold", "Silver" ,"Copper"};
        int cost = 0;

        m_name.Display();
        m_description.Display();
        m_potency.Display();

        for (short i = 0; i < 4; i++)
        {
            cost = ConvertCost(i);

            if (cost != 0)
            {
                cout << ConvertCost(i) << ' ';
                Currency[i].Display();
            }
        }
    }
private:
    String m_name;
    String m_description;
    String m_potency;
    String m_cost;
};

I'm sorry it's really long and I left out some of the output formatting, but I really need some help with this.

P.S. My professor wants us to convert the cost from a string to an int and it has to be put in that "0.0.0.0" format. That's why it is written like that.

Answer 1

Your assignment operator is broken, as it copies the pointer from the right hand side.
You need to follow the same procedure as in the copy constructor.

By the way, the copy constructor is also broken, as it will fail if rhs is empty.

(It's a safer idea to use an empty string in your representation of an empty string (i.e. use "" instead of nullptr).
It saves you from a lot of null checking.)

This constructor is also broken:

String(char * str)
{
    if (str != nullptr)
    {
        m_str = new char[strlen(str) + 1];
        strcpy(m_str, str);
    }
}

as it leaves m_str uninitialised if you pass it nullptr - and you do that a lot.

This constructor is also broken:

String(char chr) : m_str(nullptr)
{
    m_str = new char;
    *m_str = ch;
}

as it doesn't construct a zero-terminated string.

You need

m_str = new char[2];
m_str[0] = ch;
m_str[1] = 0;

Answer 2

As others stated, your constructors are broken. In addition to this, the assignment operator is also broken in a subtle way.

The easiest way to fix this is move most of the assignment operator code to the copy constructor.

String(const String & copy) : m_str(new char[strlen(copy.m_str)+1])
{ 
   strcpy(m_str, copy.m_str);
}

As to your assignment operator, the flaw is it deletes the memory before new[] is called. What happens if new[] throws an exception? You've destroyed your old data and you have no way to recover the data.

Instead of how you wrote the assignment operator, a better way is this:

   #include <algorithm>
   //...
   String& operator=(String rhs)
   {
     std::swap(rhs.m_str, m_str);
     return *this;
   } 

That was simple, and it works. It requires a working copy constructor, and working destructor for String. Since you provided these two functions, writing the assignment operator becomes trivial.

See the copy/swap idiom: What is the copy-and-swap idiom?

This also ensures that if new[] throws an exception, your original string is not destroyed. The std::swap merely swaps the two items -- if you can't use std::swap then write your own function to swap the pointers, as it should be very simple to implement.