Copy Constructor containing pointer

[PapaBizz]

Hello!

I am trying my best to explain the situation, so heres my problem.

I want to copy over values from x to y, and its not possible as i cant use "y" as a index in my pointer array.

Code:

CellularPhoneStock::CellularPhoneStock(CellularPhoneStock &origObj){
	this->phonez[nrOfPhones]->setColour = origObj.getColour();
	/*this->model = origObj.model;
	this->nrOfType = origObj.nrOfType;
	this->price = origObj.price;*/
	nrOfPhones++;
}

My questions is, how can I use origObj and phonez combined to use the function getColour() ?

Thanks!!!

Btw, phones = CellularPhone * phonez[30];

[dhayden]

Please post the class declarations for CellularPhoneStock and CellularPhone.

[PapaBizz]

Thanks for your reply!

removing this codes since it isnt needed I guess :)

[dhayden]

Sorry, that isn't what I was looking for. Please post CellularPhone.h and CellularPhoneStock.h

Looking at the code, it seems that CellularPhoneStock is something like:

class CellularPhoneStock {
public:
    CellularPhone **phonez;
    unsigned nrOfPhones;
};

It looks like phonez points to an array of CellularPhone*. But I don't see you allocate that array anywhere. Also I don't see you deleting the memory that gets allocated in addPhone.

If the assignment allows it, I suggest that you use a vector of CellularPhones instead. That way all the memory management will be handled for you:

class CellularPhoneStock {
public:
    std::vector<CellularPhone> phonez;
};

[PapaBizz]

My assignment does not allow any vectors.

[dhayden]

Okay, so you have to do the memory management yourself. To do this you need to clearly define who "owns" the memory that you allocate. This usually boils down to who will delete it. In your case it's pretty clear that phonez owns the CellularPhones that it points to. That means the destructor has to delete them. I think you have that, but it looks like you deleted the post that had the code.

Now what about the copy constructor? Since phonez owns the items it points to, you have to make new copies of the source's CellularPhones. In other words, the copy constructor should go through origObj.phonez, make a copy of each CellularPhone in it, and add the copy to the this->phonez. Pay attention to exactly how many items in origObj.phonez you copy, and set this->nrOfPhones accordingly.

[PapaBizz]

Thaank you ! I got it working yaaaaay :D

Another question, since you make it seem so easy lol, the operator <, I have to overload it.

This one:

bool operator<(CellularPhone &otherObj);

Now, in the header above, its placed in the wrong class, but ive fixed it.

So what I have to overload the operator for is because I have to check wich phone has the most nrOfType (numbers of x phone), and to do so, my teacher said I have to add up the amount of nrOfPhones before returning true (I dont understand what he meant, but this is what I wrote)
:

	for (int i = 0; i < this->nrOfPhones; i++){
	if (this->nrOfType()>sec.this->nrOfType()){
	return true;
	}
	else{
	return false;
	}
	}
}

[dhayden]

So this is for comparing CellularPhones right? If you want to sort them based on nrOfType then it's:

bool CellularPhone:: operator<(const CellularPhone &right) const
{
    return nrOfType < right.nrOfType;
}

Note the two const qualifiers. You'll need these in the declaration too.

[PapaBizz]

All working! You are great :D
Thank you so so so much.

Altho I just noticed I have a small problem, not syntax related.

This is my declaration of the == overloaded operator.

bool CellularPhone:: operator==(const CellularPhone &otherObj) const{
	return this->colour == otherObj.colour && this->model == otherObj.model;
}

Now, when I use my function addPhone, I am asking it to check IF there are any duplicates, and if there is, the added phone should be deleted!

But it does not seem to work. NO phone seems to get added :/ Because the if statement always seems to be true.

here is the addphone:

void CellularPhoneStock::addPhone(string model, string colour, int nrOfType, double price){
	/*if (checkForDuplicates(model,colour) != true){
		this->phones[nrOfPhones] = new CellularPhone(model, colour, nrOfType, price);
		nrOfPhones++;
		sortPhoneStock();
	}*/
	int pos;
	this->phones[nrOfPhones] = new CellularPhone(model, colour, nrOfType, price);
	nrOfPhones++;
	sortPhoneStock();
	for (int i = nrOfPhones; i < nrOfPhones; i--){
		pos = nrOfPhones--;
		if (this->phones[pos] == phones[i] ){
			nrOfPhones--;
			delete phones[nrOfPhones];
		}
	}
}

[coder777]

But it does not seem to work. The duplicate phones still get added...

You compare pointers which are different in any case since you create a new pointer on line 8. You need to compare the content

if (*(this->phones[pos]) == *(phones[i]) )

Instead of add, sort, delete better compare the items before adding

[PapaBizz]

coder777

But in the overloaded == operator, I told it to grab the content and compare it, I thought sending the adress was the correct way of doing so, but then, now when I think about it, the adress of the pointer will be sent ?

Anyways, I did as u said, and, it adds the phones, BUT, it still add duplicates :(

[dhayden]

Your operator==() takes a reference to a CellularPhone as the parameter, so coder77 is right, you need to compare the values as he's shown.

But your loop to find a duplicate isn't right. Why are you changing nrOfPhones in there? You should do this intstead:
- Create the new CellularPhone and assign it to a temp pointer
- Go through the existing phones to see if this is a duplicate. If so then
delete the newly created CellularPhone and return.
- If you don't find a duplicate then add the new CellularPhone to the list and ONLY THEN increment nrOfPhones.

This way nrOfPhones will actually reflect the number of valid items all the time.

Now let's talk about operator==(). You've coded it to compare the color and model, but operator<() ignores the color and model and compares the nrOfType right? So consider two phones:
CellularPhone a: color=black, model=Razor, nrOfPhone = 12
CellularPhone b: color=black, model=Razor, nrOfPhone = 13
CellularPhone c: color=blue, model=iPhone, mrOfPhone = 12;

With your < and == operators:
a == b, and a < b
a != c but a<c is false, and so is c<a, which implies that they are equal

You need to ask yourself how the phones are ordered and what makes two phones equal. Then code operator< and operator== to be consistent.

[PapaBizz]

Okay.

So I made this code.

	int checker = 0;
	CellularPhone *a;
	a = new CellularPhone(model, colour, nrOfType, price);
	for (int i = 0; i < nrOfPhones; i++){
		if (*(a) == *(phones[i])){
			delete a;
			checker = 1;
		}
	}
	if (checker == 0){
		this->phones[nrOfPhones] = new CellularPhone(model, colour, nrOfType, price);
		nrOfPhones++;
	}

now, it is crashing when I try to add a phone.

if I remove the *from (a) and phones[i], it doesnt crash

EDIT: FIXED IT, removed delete a :D THANK YOU SO MUCH GUYS, I AM NOW DONE <3 LOVE THIS FORUM <3


All that is left is now to look why im getting memory leaks :D

EDIT v2:

Fixed memory leaks, added the delete a, but at the very end :)

THANK YOU GUYS SOOO MUCH

[dhayden]

I think you should keep the delete a statement and change line 11 to this->phones[nrOfPhones] = a;

If you're getting crashes etc. then you may not be setting nrOfPhones right somewhere else. Feel free to post all the code so we can look at it.

[PapaBizz]

I edited my post above, not sure why you cant see it, but here it is ( i ifxed it :D ):

EDIT: FIXED IT, removed delete a :D THANK YOU SO MUCH GUYS, I AM NOW DONE <3 LOVE THIS FORUM <3


All that is left is now to look why im getting memory leaks :D

EDIT v2:

Fixed memory leaks, added the delete a, but at the very end :)

THANK YOU GUYS SOOO MUCH