Kritik und Optimierungen (Klassen mit Operatoroverloading)

[GPC]

BorisDieKlinge schrieb:

auser das er nich von MFC ist?

Das zieht den Vorteil nach sich, dass der std::vector überall vorhanden ist, wo eine C++ Standardlib vorhanden ist... während die MFC halt (a) Win-only ist und (b) auch da erst käuflich erworben werden muss.

[BorisDieKlinge]

ok gut kann man auc hauf einen schlage die elemente von vetor a in vector b anhängen?

[Konrad Rudolph]

BorisDieKlinge schrieb:

ok gut kann man auc hauf einen schlage die elemente von vetor a in vector b anhängen?

Ja, natürlich. Schau Dir doch einfach mal in einer Doku die Methoden und Operatoren von 'std::vector' an. Das Anhängen geht mit op+= bzw. 'insert'.

[Paddy82]

Man kann auch einfach einen vector mit smart-pointern (z.B. shared_ptr von Boost) verwenden, dann ist die Speicherverwaltung auch gleich vernünftig.

Der boost::shared_ptr hat einen Referenzzähler, d.h. bei jedem Kopieren des Zeigers, wird der interne Referenzzeiger um 1 erhöht. bei jedem delete um 1 verringert. Wenn der Referenzzeiger 1 ist und man delete aufruft, wird der Speicher tatsächlich freigegeben.

Somit kann man den Zeiger im Vector speichern. Wenn der Vector gelöscht wird, wird das delete von dem im Vector gespeicherten Element aufgerufen, aber wenn man noch irgendwo anders eine Referenz auf diesen Zeiger hat, kann man später (also nach dem Löschen des Vectors) damit weiterarbeiten, da ja nur der Referenzzähler heruntergezählt wurde (Der Zeiger ist also noch gültig).

Ein anderes Problem ist, dass der Vector bei "normalen" Zeigern einfach nur den Zeiger, nicht aber das Objekt löscht (Speicherleck)

Gruß Paddy

[BorisDieKlinge]

Ja gut das war bei CTypePtrArray auch immer so. Aber das hab ich bisher noch hinbenkommen, zudem verwende ich Pointer Array auch nur als referenz speicher , die objekte udn desen Zeiger sind oft in einem extra Array gespeichert, das die objetk auch wieder löscht!!

[Gast]

In deinem letzten Code ändert op+ this, was es natürlich nicht soll und gibt auch noch keine neues Objekt zurück. ( Vllt. hast du es ja auch schon geändert ).

Zudem habe ich das Wort Polymorphie hier gehört ->

KasF schrieb:

2.) Wenn die Klasse als Basisklasse benutzt wird und Polymorphie auch mit im Spiel ist dann Destruktor virtual machen.

Und das hier:

CRoute& CRoute::operator=(/*const*/ CRoute &oScr){
            if(&oScr== this)
                return *this;
            ASSERT(&oScr!=NULL);
            this->m_paNode.RemoveAll();
            for(int i=0; i< oScr.GetSize(); i++)
                this->m_paNode.Add(&(oScr[i]));
            return *this;
        }

(Eine Refernez muss sich auf ein Objekt beziehen, kann also nie NULL sein)

KasF schrieb:

6.) op= Exceptionsicher machen, dh das Objekt erst ändern wenn alle Aufgaben erfolgreich erledigt wurden. Zuerst nen Temporäres Objekt erzeugen und dann swap und Parameter als const& wieder, oder direkt per Value und dann swap.

besser so:

CRoute& CRoute::operator=(const CRoute &oScr){
            CRoute temp(oScr);
            std::swap(temp,*this);
            return *this;
        }

[BorisDieKlinge]

Exceptionsicher heist in dem fall:

statt driekt:

std::swap(oScr,*this);

über Temp objekt:

CRoute temp(oScr);
std::swap(temp,*this);

Aber den Sinn versteh ich nich, was kann passieren wenn ich es direkt mache?

Auser wenn ich in in parallen trhead das oScr ändere, während der in op= ist?

Soll man dann die Temp des Typs auch beim Kopiekonstruktor, bzw. überall wo der typ selber als Parameter übergeben wird anlegen?

Hier der neue code CRoute mit vector:

class CRoute{

		std::vector<CGroup*> m_paNode;
		int m_i;	//DEBUG

	public:
		//Konstruktoren
		CRoute(CGroup *p){
			m_paNode.push_back(p);
		}
		CRoute(){
			//Kein Tiefes löschen notwendig da nur Referenzen
			m_paNode.clear();
		}
		//Kopiekonstruktor
		CRoute(const CRoute &oScr){
			m_paNode= oScr.m_paNode;
		}
		//Destruktor
		~CRoute(){
			m_paNode.clear();
		}
		//Anzahl der knoten in einer Route
		int GetSize() const{ 
			return 	m_paNode.size();
		}
		//Zugriff auf elemente der Route über Index		
		CGroup& CRoute::operator[](const size_t iIndex) const{
			return *(m_paNode.at(iIndex));
		}
		//Route an Route anhängen!
		void CRoute::operator+(const CRoute &oScr){
			CRoute temp(oScr);
			//Anhängen
			m_paNode.insert(m_paNode.end(),temp.m_paNode.begin(),m_paNode.end());
		}
		//Knoten an Route anhängen
		void CRoute::operator+(CGroup *&p){
			m_paNode.push_back(p);
		}
		/*Route einer anderne Route zuweisen (Tiefe Kopie nich notwendige, da die Routen nur auf
		Knoten des Baumen zeigen*/
		CRoute& CRoute::operator=(const CRoute &oScr){
            CRoute temp(oScr);
            std::swap(temp,*this);
            return *this;
        } 
		//Route an vorhanden Route anfügen
		CRoute& CRoute::operator+=(const CRoute &oScr){
			if(&oScr==NULL)
				return *this;
			CRoute temp(oScr);
			//Anhängen
			m_paNode.insert(m_paNode.end(),temp.m_paNode.begin(),m_paNode.end());
			return *this;
		}
		//Route an vorhanden Route anfügen
		CRoute& CRoute::operator+=(/*const*/ CGroup &oScr){
			if(&oScr==NULL)
				return *this;
			m_paNode.push_back(&oScr);
			return *this;
		}
		//Route ausgeben
		void DEBUG_TRACEOUT(){
			TRACE("Pfad ADR.: %i\n",(int)this);
			for(int i=0; i< this->GetSize(); i++)
				TRACE("ADR.: %i NODE: %s\n", (int) m_paNode.at(i), m_paNode.at(i)->GetName()); 
		}
	};

Noch mehr Optimierung möglich oder Kritik? Das Wissen kann ich dann auf die anderen Klassen ableiten;)

[rüdiger]

BorisDieKlinge schrieb:

Aber den Sinn versteh ich nich, was kann passieren wenn ich es direkt mache?

Siehe GotW die Kapitel über Exception-Sicherheit

class CRoute{

Nimm _kein_ C als Prefix für deine Klassen! Das C als Prefix wurde von der MFC eingeführt, weil es damals noch keine Namespaces gab, um Namenskollisionen zu vermeiden. Wenn du nun auch ein C als Prefix verwendest, dann zerstörst du das Konzept und riskierst potentielle Namenskollisionen! Außerdem machst du den Code unleserlich (Wahllos Buchstaben irgend wo anfügen macht Namen nicht gerade einprägsammer!)

int GetSize() const{ 
			return 	m_paNode.size();
		}

schau mal was std::vector<T>::size zurück gibt. Es gibt _kein_ int zurück. Daher solltest du das auch nicht machen!

//Zugriff auf elemente der Route über Index		
		CGroup& CRoute::operator[](const size_t iIndex) const{
			return *(m_paNode.at(iIndex));
		}

das const in der Parameterliste ist sinnlos. Außerdem sollte die Methode nicht const sein, wenn du eine veränderbare Referenz zurück gibst.

//Route an Route anhängen!
		void CRoute::operator+(const CRoute &oScr){
			CRoute temp(oScr);
			//Anhängen
			m_paNode.insert(m_paNode.end(),temp.m_paNode.begin(),m_paNode.end());
		}

operator+ sollte kein Member sein! (Siehe GotW)

//Route an vorhanden Route anfügen
		CRoute& CRoute::operator+=(const CRoute &oScr){
			if(&oScr==NULL)
				return *this;

Was ist der Sinn dieses Vergleichs? Referenzen sind _nie_ NULL, außer jemand treibt bewusst Schabernack

Generell empfehle ich dir mal ein Blick in Effektiv C++ und GotW. Du wiederholst viele der üblichen Standard Fortgeschrittenen Fehler, die in den Texten einem ziemlich gut ausgetrieben werden.

[Gast]

BorisDieKlinge schrieb:

Exceptionsicher heist in dem fall:

statt driekt:

std::swap(oScr,*this);

über Temp objekt:

CRoute temp(oScr);
std::swap(temp,*this);

Aber den Sinn versteh ich nich, was kann passieren wenn ich es direkt mache?

Der Sinn ist es dein Objekt zu sichern, damit es im Fehlerfalle nicht "kaputt" geht.
Bsp:

CRoute& CRoute::operator=(/*const*/ CRoute &oScr){
            if(&oScr== this)
                return *this;
            ASSERT(&oScr!=NULL);
            this->m_paNode.RemoveAll();        //(1)
            for(int i=0; i< oScr.GetSize(); i++)
                this->m_paNode.Add(&(oScr[i]));
            return *this;
        }

(1) Hier säuberst du ja this um das neue Objekt reinzustecken.
Sagen wir das hat geklappt. Aber dann wirft zB Add(...) ne Exception.
Was nun:

CRoute a,b;
//...
a = b; // Exception
// Autsch mein A ist kaputt und die Zuweisung hat auch nicht funktioniert

Ne Regel besagt das du deine Objekte in nem sicheren Zustand solange wie möglich beibehalten sollst. Wieso es schon ändern, falls es gar nicht geändert werden kann.

Schauen wir uns den neuen Code an:

CRoute temp(oScr);
std::swap(temp,*this);
return *this;

Wo kann den hier ne Exception auftreten ? Nur bei der Erzeugung von temp! Und das ist nicht schlimm, da es meinem this noch gut geht. Swap wirft eh keine Exception
Das einzige wo hier noch was passieren könnte wäre beim Rückgabewert, aber das ist ne andere Geschichte ...

Wieso nicht direkt ???

std::swap(oScr,*this);

Ganz einfach.
1. Dein oScr ist ein const CRoute&, also kann man es schonmal nicht ändern.
2. Wenn es nicht const wäre auch böse, denn du würdest oScr ändern und der Inhalt vom this würde drin stehen:

CRoute a,b;

a = b; 
// Yuhuu mein a ist nun ein b
// Ohh!! Mein b ist ein a *confuse* <- nicht sinn einer zuweisung !

Zudem gelangt bei dieser Variante der Inhalt von this hier:

std::swap(temp,*this);

in temp und wird am Ende schön aufgeräumt

[BorisDieKlinge]

Das nenn ich ne Erklärung;)hehe danke;)