Deleting dynamic array using class destructor

Question

I've got a simple program that stores sets of 3 variables using two classes, one called Stock which has the 3 variable definitions, along with basic inspector and mutator functions, and a second function called StockManagement that has a pointer to Stock and some other functions that do all sorts of things such as creating a dynamic array of Stock, dumping a files data into the array we just made and so on and so forth.

Everything works fine in terms of creating the stuff (such as the constructor, array, entering data, reading data) but when it comes to the destructor, there is a chance that SOMETIMES the program will seg fault at the delete command, and the times it doesn't, the data is still accessable for some reason or another.

Here's what I believe to be the important parts of the code

Header file

class Stock {
private:
    char tag[5];
    int cost;
    long volume;

public:
    void setTag(char);
    void setCost(int);
    void setVolume(long);

    char* getTag();
    int getCost();
    long getVolume();

}

class StockManagement {
private:
    Stock* data;
    int size;

public:
    StockManagement();
    ~StockManagement();

   // other functions
}

Cpp file (the proper name escapes me)

...

void Stock::setTag(char tag) {
    this->tag = tag;
}

void Stock::setCost(int cost) {
    this->cost = cost;
}

void Stock::setVolume(long volume) {
    this->volume = volume;
}

char* Stock::getTag() {
    return this->tag;
}

int Stock::getCost() {
    return this->cost;
}

long Stock::getVolume() {
    return this->volume;
}

// --

StockManagement::StockManagement() {
    data = NULL;
    size = 0;
}

StockManagement::~StockManagement() {
    if (data != NULL) {
        std::cout << data[0].getTag() << std::endl; // Debug line
        delete [] data;
        std::cout << data[0].getTag() << std::endl; // Debug line

        data = NULL;
        size = 0;
    }
}

void StockManagement::allocateMemory() {
    data = new Stock[size];
}

...

So at the point in time when calling the destructor, there is always data in there. Theres a function always called that gets the amount of lines from a file (and saves it as size) which is used to allocate the memory, in which data is then entered into the array.

Then comes time to use the destructor. The first cout line will always output, as expected. Then theres a chance that either two things happen from then on. We seg fault at the delete line, or we go over it, calling the second cout and somehow printing the same data we printed the first time.

So obviously the data wasn't deleted. But why? And why does this only happen sometimes and other times it just straight up seg faults?

Answer 1

You have this for your constructor and destructor.

StockManagement::StockManagement() {
    data = NULL;
    size = 0;
}

StockManagement::~StockManagement() {
    if (data != NULL) {
        std::cout << data[0].getTag() << endl; // Debug line
        delete [] data;
        std::cout << data[0].getTag() << endl; // Debug line

        data = NULL;
        size = 0;
    }
}

As there is nothing wrong with your constructor but as a personal preference I prefer this:

StockManagement::StockManager() :
  data( nullptr ),
  size( 0 )
{}

For your destructor I see a problem within the code

StockManagement::~StockManagement() {
    if ( data != NULL ) {
        std::cout << data[0].getTag() << endl; // Debug Line
        delete[] data;
        std::cout << data[0].getTag() << endl; // Debug Line

        data = NULL;
        size = 0;
    }
 }

I see a few errors and can optimize for easier reading. Your first error is that you are using std::cout so I am assuming you do not have using namespace std; defined. This means that you will also need to use the scope resolution operator as well for endl; you need to have it declared as ... std::endl;

Your second error is after you call delete [] data; you then on the next line are trying to access the element at index 0 of data and calling a getTag() method. This is undefined behavior and can cause crashes, unhandled exceptions etc.

As for readability, your naming conventions in you class members could use a little bit of work. Also one thing to think of is for any pointer member in your class you can use either std::unique_ptr<ClassType> or std::shared_ptr<ClassType> depending on the need. If this class has sole ownership of the object and you do not want any outside object to modify it then use unique but if you need other objects to work with this data member, modify then the class saves the modified data you can use shared. The beauty of unique & shared is when the object goes out of scope you do not have to worry about calling delete or delete[] on that object and you are preventing possible memory leaks. I would also replace NULL for any raw pointer with nullptr if your compiler supports it, but most if not all modern compilers should already support this by now. Instead of having this in your class declaration if you plan on using the new and delete | delete [] operators.

private:
    Stock* data;

I would suggest this

private:
    std::shared_ptr<Stock> data;
    // Or
    std::unique_ptr<Stock> data;

I hope this helps you out.

Edit:

I also noticed that in your StockManagement class you define a pointer to a Stock object which is a class object, you then initialize it to be NULL or a nullptr in your constructor, but within your destructor you are calling delete [] on data. Normally delete [] is used on an array of pointers which I do not see defined in your code. You do have a member function to allocate memory using new with the bracket operator but you have only defined your member variable as a pointer to an object, not a pointer to a pointer of objects.