Destructor problems+ list display

Question

I have the following code: The problem is when I create a list in main of type: Reteta. After I display the list I receive am error of bad allocation. If I comment the destructor from the class Reteta the program works. Can you help me find the bug? Or maybe I didn't display the list well so the program have other problems to take care of. Here is the code:

#include<iostream>
#include<fstream>
#include<list>
using namespace std;

class Medicament{
private:
    char *denumire;
    float pret;
public:
    Medicament()
    {
        this->pret = 0;
        this->denumire = new char[strlen("Fara denumire")];
        strcpy(this->denumire, "Fara denumire");
    }
    Medicament(char* denumire, float pret)
    {
        this->denumire = new char[strlen(denumire) + 1];
        strcpy(this->denumire, denumire);
        this->pret = pret;
    }
    Medicament(const Medicament& x)
    {
        this->denumire = new char[strlen(x.denumire) + 1];
        strcpy(this->denumire, x.denumire);
        this->pret = x.pret;
    }
    ~Medicament()
    {
        if (this->denumire)
        {
            delete[] this->denumire;
        }
    }
    void setDenumire(char *x)
    {
        if (x)
        {
            if (this->denumire)
            {
                delete[] this->denumire;
            }
            this->denumire = new char[strlen(x) + 1];
            strcpy(this->denumire, x);
        }
    }
    char* getDenumire()
    {
        return this->denumire;
    }
    void setPret(float f)
    {
        if (f)
        {
            this->pret = f;
        }
    }
    Medicament operator=(Medicament x)
    {
        this->denumire = new char[strlen(x.denumire) + 1];
        strcpy(this->denumire, x.denumire);
        this->pret = x.pret;
        return *this;
    }
    friend ostream& operator<<(ostream& consola, Medicament &x)
    {
        consola << "Medicament: " << x.denumire << endl; //error here
        consola << "Pret: " << x.pret << endl;
        return consola;
    }
    float getPret()
    {
        return this->pret;
    }
    friend class Reteta;
};

class Reteta{
protected:
    Medicament *medicamente;
    int n;
public:
    Reteta()
    {
        this->n = 0;
        this->medicamente = NULL;
    }
    Reteta(Medicament *v, int n)
    {
        this->n = n;
        this->medicamente = new Medicament[n];
        for (int i = 0; i < n; i++)
        {
            this->medicamente[i] = v[i];
        }
    }
    ~Reteta()
    {
        if (this->medicamente)
        {
            delete[] this->medicamente;   //The problem is here. If I comment this the program works.
        }
    }
    int getN()
    {
        return this->n;
    }

    friend ostream& operator<<(ostream& consola, Reteta& x)
    {
        consola << "Numar de medicamente: " << x.n << endl;
        consola << "    -->Lista Medicamente<-- "<<endl;
        for (int i = 0; i < x.n; i++)
        {
            consola << x.medicamente[i].getDenumire() <<endl; //error at this line when I compile
            consola << x.medicamente[i].getPret()<< endl;

        }
        return consola;
    }
    void adaugaMedicament(Medicament x)
    {
        Reteta y;
        y.medicamente= new Medicament[this->n+1];
        for (int i = 0; i < this->n; i++)
        {
            y.medicamente[i] = this->medicamente[i];
        }
        y.medicamente[this->n] = x;
        delete[] this->medicamente;
        this->medicamente = new Medicament[this->n + 1];
        this->n++;
        for (int i = 0; i < this->n; i++)
        {
            this->medicamente[i] = y.medicamente[i];
        }
    }
    Medicament operator[](int i)
    {
        if (i >= 0 && i < this->n)
        {
            return this->medicamente[i];
        }
    }
    friend class RetetaCompensata;
    virtual float getValoare()
    {
        float sum = 0;
        for (int i = 0; i < this->n; i++)
        {
            sum=sum+this->medicamente[i].getPret();
        }
        return sum;
    }
    friend ifstream& operator>>(ifstream& consola, Reteta& x)
    {
        char aux[30];
        float z;
        consola >> x.n;
        if (x.medicamente)
            delete[] x.medicamente;
        x.medicamente = new Medicament[x.n];
        for (int i = 0; i < x.n; i++)
        {
            consola >> aux >> z;
            Medicament m(aux, z);
            x.medicamente[i] = m;
        }
        return consola;
    }
};

class RetetaCompensata : public Reteta{
private:
    float procentCompensat;
public:
    RetetaCompensata(float procent)
    {
        this->procentCompensat = procent;
    }
    RetetaCompensata(Reteta r, float procent)
    {

        this->procentCompensat = procent;
        this->n = r.n;
        this->medicamente = new Medicament[r.n];
        for (int i = 0; i < r.n; i++)
        {
            this->medicamente[i] = r.medicamente[i];
        }
    }
    float getValoare()
    {
        float sum = 0;
        sum = this->procentCompensat*this->getValoare();
        return sum;
    }

    friend ostream& operator<<(ostream& consola, RetetaCompensata &x)
    {
        consola << "**Procent compensat: " << x.procentCompensat << endl;
        consola << "Numar de medicamente: " << x.n << endl;
        consola << "    -->Lista Medicamente<-- " << endl;
        for (int i = 0; i < x.n; i++)
        {
            consola << x.medicamente[i] << " ";
        }
        return consola;
    }
};

void main()
{
    //1
    Medicament nurofen("Nurofen", 11.25f);
    Medicament aspirina = nurofen;
    aspirina.setDenumire("Aspirina");
    aspirina.setPret(4.5f);
    Medicament bixtonim("Bixtonim", 8.2f);
    Medicament temp;
    temp = nurofen;
    cout << temp << endl;
    cout << nurofen << endl;
    cout << aspirina << endl;
    //2
    Medicament medicamente[] = { aspirina, nurofen };
    Reteta r0(medicamente, 2);
    cout << r0 << endl;
    //3
    Reteta r1;
    r1.adaugaMedicament(nurofen);
    r1.adaugaMedicament(aspirina);
    for (int i = 0; i < r1.getN(); i++)
    {
        cout << r1[i] << endl;
    }
    //4
    RetetaCompensata r2(0.5);
    r2.adaugaMedicament(bixtonim);
    r2.adaugaMedicament(aspirina);


    RetetaCompensata r3(r1, 0.2);
    cout << "AFISARE R3" << endl;
    cout << r3 << endl << endl;
    Reteta* p = &r1;
    cout <<"Valoare reteta r1: "<< p->getValoare() << endl;
    //5
    Reteta r4;
    ifstream fisier("retete.txt");
    fisier >> r4;
    cout << r4 << endl;
    //6
    cout << endl << "Afisare Lista :" << endl << endl << endl;
    list<Reteta> R;
    list<Reteta>::iterator it;
    R.push_back(r0);
    R.push_back(r1);
    R.push_back(r3);
    R.push_back(r2);
    R.push_back(r4);
    for (it = R.begin(); it != R.end(); it++)
    {
        cout << *it << "  Valoare Reteta: " << it->getValoare() << endl << endl << endl; // error at this line when I compile 
    }
}

`Medicament operator=(Medicament x)` The assignment operator should be returning a reference, not a new object. And why are you not using `std::string` instead of `char *`? Doing that alone cuts out major portions of the code you've written. — PaulMcKenzie, Feb 07 '16 at 00:09
I'm a newbie. I have to do this. In some weeks I will play only with STL. — Ciobanu Rares-Constantin, Feb 07 '16 at 00:14
It should be the opposite. Use the STL now, use the pointers later (much later). — PaulMcKenzie, Feb 07 '16 at 00:14
When your class allocated memory in the constructor and deallocates the memory in the destructor, you will have to implement the copy constructor and the copy assignment operator properly. — R Sahu, Feb 07 '16 at 00:15
Hmm.. You're using `std::list` but not `std::string`? Seriously, all of this code would work if you used `std::string` instead of `char *`, and `std::vector` instead of `new Medicament[n]`. — PaulMcKenzie, Feb 07 '16 at 00:24

PaulMcKenzie · Accepted Answer · 2016-02-07T00:24:19.350

0

This is a memory overwrite:

    this->denumire = new char[strlen("Fara denumire")];
    strcpy(this->denumire, "Fara denumire");

You are not allocating room for the terminating null character:

    this->denumire = new char[strlen("Fara denumire") + 1];
    strcpy(this->denumire, "Fara denumire");

But why do this when you have std::string available? That alone not only alleviates errors like this, but you don't need to write assignment operators, copy constructor, or destructor for your Medicament class.

The other error is that your Reteta class lacks a copy constructor and assignment operator, thus it is not safely copyable due to the Medicament* member. You are then using this class as a type in std::list<Reteta>.

Since Reteta is not safely copyable, and std::list makes copies, you enter the world of undefined behavior. Thus you must provide appropriate copy / assignment operators for the Reteta class.

edited Feb 07 '16 at 00:24

answered Feb 07 '16 at 00:13

PaulMcKenzie

34,698
4
24
45

Blame my teacher :)) in 30h I have my finals at PO. So we are obliged to use *char etc. STL is like 10% of exam. So what is the error? If I remove the destructor from the class Reteta it works.. – Ciobanu Rares-Constantin Feb 07 '16 at 00:18
Line 116 and 265 are the problems – Ciobanu Rares-Constantin Feb 07 '16 at 00:24
"Since Reteta is not safely copyable, and std::list makes copies, you enter the world of undefined behavior. Thus you must provide appropriate copy / assignment operators for the Reteta class." This was my problem. I created these operators and worked. Thank you for your time Paul! – Ciobanu Rares-Constantin Feb 07 '16 at 00:32

Destructor problems+ list display

1 Answers1