dynamic array and losing data of the first element

Hello!

Here's my problematic code:

Code:

class konto
{
private:
	char *nazwa;
	int size;
	papier *tabl;
public:
	~konto();
	konto(char *);
	void dodajPapier(papier &);
	void zapiszWplik(const char *nazwaPliku);
};

konto::konto(char *nazwa)
{
	this->nazwa = new char [len(nazwa)];
	strcpy(this->nazwa, nazwa);

	this->size = 0;
	tabl = new papier[size+1];
}

konto::~konto()
{
	delete [] tabl;
}

void konto::dodajPapier(papier &akcja)
{                                                    //first element in the array still exists
	tabl[size] = akcja;          
                                                     //here tabl[0] points somewhere else but not to 0-element

	papier *temp = new papier[++size];

	for(int i = 0; i < size; i++)
		temp[i] = tabl[i];

	tabl = temp;

}

When I add second object of papier the value of tabl[0] is lost.. Next objects are created properly in the array.
Why's that ? I can't fix it even though I know where it happens :-/

At a glance, it seems that:

• Your destructor does not take care of nazwa.
• Your class lacks a correctly implemented copy constructor and copy assignment operator.
• The constructor should have a const char* parameter.
• dodajPapier should have a const reference parameter.
• In dodajPapier, you fail to destroy the existing array before assigning temp to tabl.
• Expanding the dynamic array each time an element is added is slow.

These problems can be solved by changing nazwa to be a std::string and tabl to be a std::vector<papier>. You can then do away with the size member variable. In the process, the problem that you observed may also disappear.

Thank you for your reply

Your class lacks a correctly implemented copy constructor and copy assignment operator.

Class papier has these:

Code:

papier::papier(const papier &akcja)
{
	this->nazwa = new char [20];
	::strcpy(nazwa, akcja.nazwa);
	this->cenaKup = akcja.cenaKup;
	this->cenaSprz = akcja.cenaSprz;
	this->ilosc = akcja.ilosc;
	this->prowizja = akcja.prowizja;
}

papier& papier::operator =(const papier &akcja)
{
	this->nazwa = new char [20];
	::strcpy(nazwa, akcja.nazwa);
	this->cenaKup = akcja.cenaKup;
	this->cenaSprz = akcja.cenaSprz;
	this->ilosc = akcja.ilosc;
	this->prowizja = akcja.prowizja;
}

papier::~papier()
{
	delete [] nazwa;
	nazwa = NULL;
}

and the default constructor also.
I think something is wrong with deleting [] tabl, but when I put it from the destructor to function dodajPapier like this:

Code:

delete [] tabl;
tabl = temp;

I get some memory crash error
ps: I followed your suggestion and fixed:

* Your destructor does not take care of nazwa.
* The constructor should have a const char* parameter.
* dodajPapier should have a const reference parameter.

Originally Posted by fx69

Class papier has these:

Good to hear, but I was talking about konto

Originally Posted by fx69

I think something is wrong with deleting [] tabl

I suspect that the problem lies with papier's copy assignment operator, e.g., it performs shallow copies when it should perform deep copies. Again, if you make use of the standard library, you can eliminate much of this error prone memory management.

I know I could use vectors and strings but I'd like to practice of pointers That's generally why I drain this code so much
What do you mean saying: 'shallow/deep copies' ?

Originally Posted by fx69

What do you mean saying: 'shallow/deep copies' ?

With respect to pointers, a shallow copy involves copying only the pointer, whereas a deep copy copies the entire dynamic array.

Forgive me for butting in but don't you only have to use "this" if you have a parameter name matching a member name?

Code:

papier::papier(const papier &akcja)
{
	delete this->nazwa;

	this->nazwa = new char [len(akcja.nazwa) + 1];
	::strcpy(this->nazwa, akcja.nazwa);

	this->cenaKup = akcja.cenaKup;
	this->cenaSprz = akcja.cenaSprz;
	this->ilosc = akcja.ilosc;
	this->prowizja = akcja.prowizja;
}

papier & papier::operator =(const papier &akcja)
{
	if(this != &akcja) 				//if object's the same: A = A
	{
		delete this->nazwa;

		this->nazwa = new char [len(akcja.nazwa) + 1];
		//for(int i=0; i<len(akcja.nazwa); i++)
		//	this->nazwa[i] = akcja.nazwa[i];

		::strcpy(this->nazwa, akcja.nazwa);

		this->cenaKup = akcja.cenaKup;
		this->cenaSprz = akcja.cenaSprz;
		this->ilosc = akcja.ilosc;
		this->prowizja = akcja.prowizja;
	}

	return *this;
}

Is this what I need ?
But still doesn't work :/

Originally Posted by fx69

Is this what I need ?

I cannot say yes or no without knowing the definition of papier. What I can say is that the copy assignment operator is not exception safe since you destroy the existing C-style string before you have created the new one (you have no guarantee that the new one can be created). You certainly do not need to destroy the existing C-style string in the copy constructor since it does not exist, and in fact the pointer may contain garbage at that point.

If you want to practice the use of manual memory management, then I suggest that you stop what you are doing now and start working on a string class and/or dynamic array of int class. Clearly, you are doing more then you can handle at the moment, i.e., you are trying to write a class that has both a dynamic string and dynamic array, and the dynamic array is of objects that are themselves written by you to use manual memory management. There are too many things that can go wrong.

You are right I just didn't think that the project would get beyond me so much..
However it's only the first element of the array which data is lost in a certain place so I'll keep on trying to figure out what's wrong.
Here's my almost whole def of class 'papier':

Code:

class papier
{
private:
	unsigned int ilosc;
	float cenaKup, cenaSprz, prowizja;
	char kierunek;
public:
	char *nazwa;				//will be private ! ! !

	//constructors
	papier (const papier &);		//copy constructor
	papier (char *, char, int, float, float, float);
	papier ();

	papier &operator=(const papier &);			//overloaded operator

	string show();
	float wartosc();
	float wartosc(float);

	~papier();
};

papier::papier()
{
	this->cenaKup = 0;
	this->cenaSprz = 0;
	this->ilosc = 0;
	this->kierunek = NULL;
	this->nazwa = NULL;
	this->prowizja = 0;
}

papier::papier(char *nazwa, char kierunek, int ilosc, float cenaKup, float cenaSprz, float prowizja)
{
	if(ilosc < 0 || cenaKup < 0 || cenaSprz < 0)
		cout << "Wartości nie mogą być mniejsze od 0\n";
	else
	{
		this->cenaKup = cenaKup;
		this->cenaSprz = cenaSprz;
		this->ilosc = ilosc;
	}

	if(len(nazwa) < 20)
	{
		this->nazwa = new char [20];
		::strcpy(this->nazwa, nazwa);
	}
	else
		cout << "Zbyt długa nazwa papieru\n";

	if(kierunek == 'K' || kierunek == 'S')
		this->kierunek = kierunek;
	else
		cout << "Błąd typu 'kierunek'\n";

	if(!prowizja || prowizja >= 1) cout << "Niepoprawna wartość prowizji!";
	else this->prowizja = prowizja;
}

papier::papier(const papier &akcja)
{
	this->nazwa = new char [20];
	::strcpy(this->nazwa, akcja.nazwa);

	this->cenaKup = akcja.cenaKup;
	this->cenaSprz = akcja.cenaSprz;
	this->ilosc = akcja.ilosc;
	this->prowizja = akcja.prowizja;
}

papier & papier::operator =(const papier &akcja)
{
	if(this != &akcja) 		//if object's not the same: {A = A}->only returns pointer
	{
		delete this->nazwa;

		this->nazwa = new char [20];

		::strcpy(this->nazwa, akcja.nazwa);

		this->cenaKup = akcja.cenaKup;
		this->cenaSprz = akcja.cenaSprz;
		this->ilosc = akcja.ilosc;
		this->prowizja = akcja.prowizja;
	}

	return *this;
}

papier::~papier()
{
	delete [] nazwa;
	nazwa = NULL;
}
//other definitions
//...

If it's (as you mentioned before) hard to tell what's wrong in here, leave it at that
I appreciate your help and concern

ps: I followed the suggestion, and expanded the array every 10 elements added. It works