C++: Dynamic memory allocation and class constructors

Question

I believe that I may be either misusing OOP or doing something wrong with my memory allocation in class constructors.

I will illustrate what I am doing with a simplified example:

class data
{
    data(int n) : m_data(new double[n])
    {
    }

    virtual ~data()
    {
        delete [] m_data;
    }

    double min() { ... }
    double max();
    double mean(); // return minimum, maximum and mean values

    double *m_data;
}

class dual_data : public data
{
    dual_data(int n) : m_data(new double[2*n])
    {
    }
}

I believe this will cause a disastrous memory leak. (It should be clear why, as one constructor allocates memory before the other then overwrites the pointer with another call to new.)

In case it isn't already clear, I have 2 classes: One is a class similar to std::vector which handles data storage in memory for me, and the other is a derived class which does the same thing, but expects data in the format of x,y pairs rather than individual values. The reason for this is that x,y pairs can be processed in different ways than to just a vector. For example, with statistical data processing to compute correlations or something else entirely. But it is still useful to be able to call the functions min(), max() and mean() in the class dual_data, which are inherited from data.

What should I do to correct for the memory leak?

Answer 1

I would approach this differently, instead declare an interface containing the min,max and mean. Then create the classes that implement the interface but with different underlying structures.

struct ICommon
{
  virtual double min() = 0;
  virtual double max() = 0;
  virtual double mean() = 0;
};

class Data : public ICommon
{...};

class DualData : public ICommon
{...};

Answer 2

First, implement a basic vector class (or use directly std::vector<double> if possible)

class vector_of_double
{
public:
    explicit vector_of_double(std::size_t mSize) :
        mValues(new double[size]{}),
        mSize(size)
    {}

    ~vector_of_double() { delete[] mValues; }

    // implement these if required
    // deleted for now to Respect rule of 3(5) as default implementation is incorrect.
    vector_of_double(const vector_of_double&) = delete;
    vector_of_double& operator = (const vector_of_double&) = delete;
    vector_of_double(const vector_of_double&&) = delete;
    vector_of_double& operator = (const vector_of_double&&) = delete;

    std::size_t size() const { return mSize; }

    // Implement required missing stuff if required as push_back, resize, empty, clear
    // introduction of mCapacity may be required.

    double operator[] (std::size_t index) const { return mValues[index]; }
    double& operator[] (std::size_t index) { return mValues[index]; }

    // To be able to use for range and iterator style
    const double* begin() const { return mValues; }
    const double* end() const { return mValues + mSize; }

    double* begin() { return mValues; }
    double* end() { return mValues + mSize; }

private:
    double* values;
    std::size_t size;
};

then you may implement your classes (without worry about memory management):

class data
{
public:
    explicit data(int n) : m_data(n) {}

    // Or use your own implementation
    // TODO: Handle case with empty vector
    double min() const { return *std::min_element(m_data.begin(), m_data.end()); }
    double max() const { return *std::max_element(m_data.begin(), m_data.end()); }
    double mean() const {return std::accumulate(m_data.begin(), m_data.end(), 0.) / m_data.size();}

private:
    vector_of_double m_data;
};

And

class dual_data
{
public:
    explicit dual_data(int n) : m_data1(n), m_data2(n) {}

    // your functions as
    std::pair<double, double> means() double {return {m_data1.means(), m_data2.means()};}

private:
    data m_data1;
    data m_data2;
};

Answer 3

You cannot initialize a variable of your parent class. It will not compile.

You can have the memory leak if you do inside the constructor.

class dual_data : public data
{
public:
    dual_data(int n) : data(n)
    {
         m_data = new double[2*n];
    }
};

However, it is your responsability do it well. A great powder is a great responsibility.

You have it doing this:

class dual_data : public data
{
public:
     dual_data(int n) : data(2*n) {}
};

A better solution is not use inheritance. It is allways better Composititon over inheritance.

#include "data.h"

class dual_data {
public:
     dual_data( int n ) : data_1(n), data_2(n) {}
private:
     data data_1;
     data data_2;
};

Better using interfaces:

class CdataIf
{
public:
     virtual ~CDataIf() = 0;
     virtual double min() = 0;
     virtual double max() = 0;
     virtual double mean() = 0;
};

class data      : public CdataIf { ... };
class data_dual : public CdataIf { ... };

Answer 4

Maybe this should do: you tell the base class to allocate an array of 2*n elements. Besides the base class is responsible for freeing the memory. No destructor needed.

class dual_data : public data
{
    dual_data(int n) : data(2*n)
    {
    }
}

Answer 5

Since data already has an array of doubles, instead of trying to muck with that, just make another parallel array of doubles in dual_data.

class data
{
  public:
    data(int n) : m_data(new double[n])
    {
    }

    virtual ~data()
    {
        delete [] m_data;
    }
    double *m_data;
}

class dual_data : public data
{
  public:
    dual_data(int n) : data(n), d_data(n)
    {
    }

    virtual ~dual_data() {
        delete[] d_data;
    }
    double* d_data;
}

Now, your dual_data class has the original m_data array, plus an additional d_data array of the same length, whose elements are to be used for the second element in the pair. For example, the zeroth pair would be (m_data[0], d_data[0]).