Keskustelu: Ohjelmointikysymykset: C++: Operaattorin ylikuormittaminen menee pieleen

Aikani tapeltuani sain koodin kääntymään eikä myöskään vääristä "korttipakkaa" laittamalla satunnaisia "kortteja" useampia kappaleita. Nyt pakka ei sekoitu laisinkaan :-(

Luulisin vian piilevän = operaattorin ylikuormittamisessa mutta kuinka tuo ylikuormittaminen sitten tulisi hoitaa?

void randomizeDeck(card *deck, int cards) {
	int a=0, b=0;
	card *c=new card();
	for (int i=0;i<1000;i++) {
		a=rand()%cards+1;
		b=rand()%cards+1;
		c=&deck[a];
		deck[a]=&deck[b];
		deck[b]=c;

	}
}

class card {
protected:
	int value;
	int land; //0=heart, 1=spade, 2=diamond, 3=club
	int image; // points to index of array with cards
public:
	card();
	card(int v, int l, int i);
	void setValues(int v, int l, int i);
	int getValue();
	int getLand();
	int getImage();
	card* operator=(card *c);
};

card* card::operator=(card *c) {
	return new card(c->getValue(), c->getLand(), c->getImage());
}

Koodissasi on kaksi virhettä. Sijoitusoperaattorisi ei toimi oikein, ja et myöskään käytä sitä oikein. Operaattorin pitäisi muuttaa korttia itseään, siis näin:

card* card::operator = (const card *c) {
    this->value = c->value;
    this->land = c->land;
    this->image = c->image;
    return this;
}

Vaikka operaattori nyt toimisikin, sijoituksesi menevät yhä pieleen. Sijoitat muuttujaan c osoitteen &deck[a]. Menetät new-operaattorilla luomasi kortin osoitteen ja muistia vuotaa. Seuraavaksi korvaat kortin deck[a] kortilla deck[b], jolloin käytetään omaa sijoitusoperaattoriasi. Lopuksi sijoitat deck[b]:n paikalle deck[a]:n, koska c nyt osoittaa siihen. Tämä ei siis muuta deck[b]:n arvoa, koska se juuri aiemmin sijoitettiin deck[a]:han.

Koodi toimisi siis näin:

card *c = new card();
*c = &deck[a];
deck[a] = &deck[b];
deck[b] = c;
delete c; // Ei muistivuotoja! Muisti pitää aina vapauttaa!

Miksi edes haluat käyttää sijoituksessa osoittimia? Yleensä viittaukset sopivat tarkoitukseen paremmin:

card const& card::operator = (card const& c) {
  value = c.value;
  land = c.land;
  image = c.image;
  return *this;
}

card c;
c = deck[a];
deck[a] = deck[b];
deck[b] = c;

Vielä parempi on vaikkapa toteuttaa luokalle swap-funktio:

#include <algorithm>
void card::swap(card& c) {
  std::swap(value, c.value);
  std::swap(land, c.land);
  std::swap(image, c.image);
}

deck[a].swap(deck[b]);

Ohjesääntö: C++-koodi on parhaimmillaan, kun siinä on mahdollisimman vähän new-operaattoreita ja yhtä monta delete-operaattoria. Kaikki new-operaattorilla varatut muistinpalat täytyy vapauttaa delete-operaattorilla. Muistivuodolla tarkoitetaan sitä, että näin ei tehdä. Tällöin varattu muisti ei vapaudu uudestaan käyttöön, vaan luotu objekti jää muistiin, kunnes ohjelma suljetaan tai vanhoissa käyttöjärjestelmissä uudelleenkäynnistykseen asti.

Alkoi toimimaan. Tosin mystinen -33686019(value) of -572662307(land) siellä oli(sekä operaattorilla tai swap funktiolla) mutta voitonpuolella jo ollaan.

Ja miksi osoittimia? Yritin viittauksia ja const sanaa ja kääntäjä veteli herneitä nenään ja pisti pitkän liudan virheilmoituksia joten päätin yrittää osoittimilla joilla koodi edes kääntyi.

Edit: Omituinen lukukin ratkesi. Liian tottunut käyttämään rand() funktiota nopanheittoon joten teinkin kortin valitsijasta 52 sivuisen nopan jossa on arvot 1-52. Sopii arvata mitä tapahtuu kun satunnaisgeneraattori tunkee luvun 52 indeksiksi 52 alkioiseen taulukkoon :oops: