Vector aus Zeigern

Michael E.

pfandflasche schrieb:

UML Diagramm:
http://imageshack.us/a/img845/2813/auktionanwendung.jpg

JPEG ist nicht so gut geeignet, scharfe Konturen beizubehalten. Für Schrift ist daher z.B. PNG besser geeignet.

Also habe mein eigentliches Problem mit der Funktion getLetztesGebot(Auktion*auktion) jetzt so gelößt:
Bid* gebot = new Bid();
   for (int i=0;i<bid_.size();i++)
   {
       if (bid_.at(i)->getAuction()==auction)
       {
           gebot=bid_.at(i);
       }
   }
   return gebot;
Der Code funzt aber ich möchte gerne wissen warum das nicht so "gut" ist, und wo der Designfehler ist.

Da liegt so einiges im Argen. Die Klasse AuctionApplication ist eine ziemlich monströse Klasse, die sehr viele Aufgaben hat: Auktionen erstellen, Bieter erstellen, einen Bieter bei einer Auktion bieten lassen, die Gebote einer Auktion speichern, das letzte Gebot einer Auktion angeben... Vielleicht hast du schonmal davon gehört, dass eine Klasse nur eine einzelne Aufgabe haben soll. Einen Bieter kann man selbst auf eine Auktion bieten lassen, d.h. die Funktionalität kommt in die Klasse Bieter. Eine Auktion kann selbst speichern, welche Gebote eingegangen sind, das braucht man nicht zentral für alle Auktionen in AuctionApplication zu speichern. Dementsprechend kann eine Auktion selbst die Frage nach dem letzten Gebot beantworten. Wenn man diesen Gedankengang fortsetzt, löst sich die Klasse AuctionApplication entweder in Luft auf oder (je nachdem, was man vorhat und wie die Umgebung aussieht) sie degeneriert zu einer zentralen Speicherstelle aller Auktionen und Bieter (aber nicht der Gebote, die sind in den Auktionen gespeichert).

Dann diese grässlichen Zeiger. Du kannst Objekte direkt im std::vector speichern und musst sie nicht mit new anlegen, um dann einen Zeiger im vector zu speichern. Dann produzierst du auch nicht solche Speicherlecks wie in der zitierten Funktion.

pfandflasche

Danke für die ausführliche Erklärung. Du hast natürlich recht, ich habe schon mal was davon gehört, dass eine Klasse nur eine Aufgabe haben soll. Jedoch war mir der "Coderahmen" hier ja vorgegeben.

Michael E. schrieb:

Dann produzierst du auch nicht solche Speicherlecks wie in der zitierten Funktion.

Kannst du mir das mit den Speicherlecks in meiner Funktion genauer erklären? Und kann ich das mit dem vorgegeben Coderahmen anders lösen, so das es nicht zu Speicherlecks kommt?

Michael E.

Zu jedem new gehört ein delete. Wo ist das delete für das new aus deiner Funktion? Du brauchst hier gar kein Objekt zu erzeugen, sondern würdest mit einem Nullpointer als Initialisierung gut fahren. Dann passt auch der Fall, dass noch überhaupt kein Gebot auf die Auktion gemacht wurde.

pfandflasche

Also so:

Gebot* AuktionAnwendung::getLetztesGebot(Auktion* auktion)
{
   Gebot* gebot =NULL ;
   for (int i=0;i<gebote_.size();i++)
   {
       if (gebote_.at(i)->getAuktion()==auktion)
       {
           gebot=gebote_.at(i);
       }
   }
   return gebot; 
}

Wenn ich den Speicher mit delete wieder freigeben will wo müsste ich es dann einfügen? Bin mir da immer unsicher.

Müsste es dann so aussehen:

Gebot* AuktionAnwendung::getLetztesGebot(Auktion* auktion)
{
   Gebot* gebot =new Gebot();
   for (int i=0;i<gebote_.size();i++)
   {
       if (gebote_.at(i)->getAuktion()==auktion)
       {
           gebot=gebote_.at(i);
       }
   }
   return gebot; 
delete gebot;

}

Swordfish

pfandflasche schrieb:

Gebot* AuktionAnwendung::getLetztesGebot(Auktion* auktion)
{
   Gebot* gebot =new Gebot(); // <- wofür brauchst du ein NEUES Gebot?
   for (int i=0;i<gebote_.size();i++)
   {
       if (gebote_.at(i)->getAuktion()==auktion)
       {
           gebot=gebote_.at(i);
       }
   }
   return gebot; // <- danach wird die Funktion verlassen.
   delete gebot; // never reached.
}

pfandflasche

Das hier kein neues Objekt gebraucht wird ist mir mittlerweile auch klar, nur wollte ich wissen, wenn ich es so machen würde wo ich das delete einfügen muss.

Ich hatte gedacht, dass wenn ich den Speicher vor dem return freigebe es zu problemen kommen kann, da ich ja einen zeiger auf diesen zurückgebe.

Oder müsste ich es rein theoretisch so machen:

Gebot* AuktionAnwendung::getLetztesGebot(Auktion* auktion)
{
   Gebot* gebot =new Gebot();
   int tmp=0;
   for (int i=0;i<gebote_.size();i++)
   {
       if (gebote_.at(i)->getAuktion()==auktion)
       {
           gebot=gebote_.at(i);
           tmp=i;
       }
   }
   delete gebot;
return gebote_.at(tmp);

}

Da fällt mir auf das ich ja die ganze Funtkion auch so schreiben kann oder:

Gebot* AuktionAnwendung::getLetztesGebot(Auktion* auktion)
{
   int tmp=0;
   for (int i=0;i<gebote_.size();i++)
   {
       if (gebote_.at(i)->getAuktion()==auktion)
       {
           tmp=i;
       }
   }
return gebote_.at(tmp);
}

Ich weiß das es sinnlos ist möchte es aber gerne verstehen, so das ich das nächste mal wenn ich es wirklich brauche das delete richtig einsetzte

Michael E.

pfandflasche schrieb:

Oder müsste ich es rein theoretisch so machen:

Gebot* AuktionAnwendung::getLetztesGebot(Auktion* auktion)
{
   Gebot* gebot =new Gebot();
   int tmp=0;
   for (int i=0;i<gebote_.size();i++)
   {
       if (gebote_.at(i)->getAuktion()==auktion)
       {
           gebot=gebote_.at(i);
           tmp=i;
       }
   }
   delete gebot;
return gebote_.at(tmp);

}

Nein, denn so löschst du das Objekt, das du zurückgibst (bzw. einen Zeiger darauf). Du müsstest im innersten Block prüfen, ob gebot noch auf dein neu erstelltes Objekt zeigt und nur genau denn delete ausführen.

Da fällt mir auf das ich ja die ganze Funtkion auch so schreiben kann oder:

Gebot* AuktionAnwendung::getLetztesGebot(Auktion* auktion)
{
   int tmp=0;
   for (int i=0;i<gebote_.size();i++)
   {
       if (gebote_.at(i)->getAuktion()==auktion)
       {
           tmp=i;
       }
   }
return gebote_.at(tmp);
}

Prinzipiell ja, nur ist 0 als Initialisierung nicht günstig, da auch an Position 0 ein Objekt steht. Stell dir vor, es gibt kein Gebot auf die Auktion. Dann wird trotzdem das erste Gebot des vectors zurückgegeben (wenn überhaupt eins existiert).

pfandflasche

Ah ja wenn es dann jemand sagt (schreibt) klingt es ganz logisch und man denkt sich warum ist man nicht selbst drauf gekommen

Vielen Dank für die Hilfe bis jetzt!

pfandflasche schrieb:

Wenn ich den Speicher mit delete wieder freigeben will wo müsste ich es dann einfügen? Bin mir da immer unsicher.

Jedesmal wenn mit new Speicher angefordert wird, muss dieser (nach Gebrauch) mit delete wieder freigegeben werden.

Das Problem mit der Aufgabe ist die Asymmetrie.
Benutzt jemand z.B. createAuction() muss diese Person im nachhinein für das delete sorgen, ohne jemals new benutzt zu haben.
Darüberhinaus steht das auch nur in der Dokumentation von createAuction() .

Es gibt wenige Funktionen, die so funktionieren, z.B. POSIXs strdup

man strdup schrieb:

NAME
strdup, strndup, strdupa, strndupa - duplicate a string

SYNOPSIS
#include <string.h>

char *strdup(const char *s);
[...]
DESCRIPTION
The strdup() function returns a pointer to a new string which is a duplicate of the string s. Memory for the new string is obtained with malloc(3), and can be freed with free(3).

Wenn man die Dokumentation liest, ist alles klar: der Zeiger der zurückgegeben wird muss ge free d werden. Man muss halt nur daran denken.
Wieder asymmetrisch.
In C, wie z.B. bei strcpy , ist die Lösung, dass der Aufrufer auch den Speicher bereitstellt - d.h. die Symmetrie ist wieder gewährleistet.

C++ bietet mit seinen Objekten und deren Konstruktoren und Operatoren Lösungen an, die viel bequemer und genauso effizient sind.
Die natürliche Implementierung von createAuction() wäre, dass ein Objekt Auction erzeugt und (per Kopie) zurückgegeben wird.

Mit diesem Wissen im Hintergrund mein Vorschlag:
Versuch dich an der Implementierung, wie die Aufgabe es vorsieht. Denk daran, dass der Aufrufer von einigen Funktionen für das freigeben des allozierten Speichers verantwortlich ist und fertig ist der Lack.
Vielleicht ist die Aufgabe eine pädagogische Finte, und im weiteren Verlauf wird darauf eingegangen.

Wenn Du ganz pfiffig bist, schreibst Du alle Pointer betreffenden Funktionen um, so dass Du z.B. unique_ptr verwendest. Dann hast Du wirklich was gelernt...und ein ziemlich praxisnahes Refactoring durchgeführt...

Go for it!

pfandflasche

Furble Wurble schrieb:

Denk daran, dass der Aufrufer von einigen Funktionen für das freigeben des allozierten Speichers verantwortlich ist und fertig ist der Lack.

Puh wie ganu wird das denn bewergstelligt.

Ich hab dann z.B. folgenden Aufruf

Auction* auction1=app->createAuction("iPhone");

muss ich dann einfach wenn auction1 nicht mehr gebraucht wird

delete auction1

aufrufen?

pfandflasche schrieb:

Furble Wurble schrieb:

Denk daran, dass der Aufrufer von einigen Funktionen für das freigeben des allozierten Speichers verantwortlich ist und fertig ist der Lack.

Puh wie ganu wird das denn bewergstelligt.

Ich hab dann z.B. folgenden Aufruf
Auction* auction1=app->createAuction("iPhone");
muss ich dann einfach wenn auction1 nicht mehr gebraucht wird
delete auction1
aufrufen?

Ähem...
Ich hab' mir das gerade nochmal angeschaut...
...und mein Posting von gestern ist für die Tonne...
Der Inhalt ist wohl richtig trifft aber hier nur bedingt zu.

Um Deine Frage zu beantworten: ja.
Allerdings solltest Du, bevor Du die Auktion löscht auch alle Gebote darauf löschen.
Und auch die zugehörigen Einträge aus den Vektoren tilgen...

Memo an mich: nach dem Ratskeller hier nicht mehr posten...

pfandflasche

Memo an mich: nach dem Ratskeller hier nicht mehr posten...

Kann ich mir denken.

Nochmal danke an alle die mir weitergeholfen haben.

Ich hab's tatsächlich mal implementiert - zu viel Zeit...

Prinzipiell ist es sehr einfach. Tatsächlich kommen nämlich nur in Auction_applications Destruktor deletes vor.