Bin am Üben, Klasse so halbwegs okay?

ZuK

Caipi schrieb:

Äh. Ich würde bevor du im Copy-Ctor den Speicher wieder neu allokierst ihn erstmal freigeben, sprich delete[] array; Oder seh ich da was falsch?

Caipi

du siehst es falsch. wir reden von einem ctor und da ist vorher noch nichts allokiert
Kurt.

Caipi

Achso

Caipi

net schrieb:

...und bei 'Grow' immer verdoppeln ist vielleicht auch nicht geschickt

Doch. Genau das ist geschickt. Ein linearers Wachstum wäre ungeschickt. Siehe dazu mal das Wachstumsverhalten von std::vector.

Bei linearem Wachstum hast Du sonst eine amortisierte lineare Laufzeit beim Hinzufügen von Elementen. Bei multiplikativem Wachstum hält man die Laufzeit in etwa konstant.

pli

int GetElementAt(int pos)const{return array[pos];}

Hier rennst du in einen Speicherfehler wenn jemand versehentlich eine Position ausserhalb des arrays nennt.

int GetElementAt(unsigned int pos)const{return array[pos<size?pos:size-1];}

So bekommst du immer ein Element aus dem Array. (Ok, nicht der beste Stil mit ?-Operator, aber funktioniert)

EDIT: Gleiches gilt fuer operator[], den sollte man auch sichern.

pli schrieb:

int GetElementAt(int pos)const{return array[pos];}
Hier rennst du in einen Speicherfehler wenn jemand versehentlich eine Position ausserhalb des arrays nennt.
int GetElementAt(unsigned int pos)const{return array[pos<size?pos:size-1];}
So bekommst du immer ein Element aus dem Array. (Ok, nicht der beste Stil mit ?-Operator, aber funktioniert)

EDIT: Gleiches gilt fuer operator[], den sollte man auch sichern.

kann ich das auch noch schoener machen?
Wobei es mir schon klar war, das wenn ich ein element abfrage welches nicht da ist, das es probs gibt!

Immer weiter her mit kritik

hallo "Hoffentlich..."
deine klasse sieht gut aus. darf ich fragen wie lange du schon C++ lernst?

hm.....so ungefähr 3monate!
Wieso?

respekt! wieso? ich lerne gerade selber, so etwa wie du oder bisschen weniger (jetzt ein buch durch) und naja, wenn es dich interessiert, dann schau hier:

http://www.c-plusplus.net/forum/viewtopic-var-t-is-107207.html

groovemaster

ZuK schrieb:

auf dem ersten blick kann ich keine gröberen fehler erkennen

Ich schon . OK, weniger Fehler, dafür mehr Unregelmässigkeiten.

@Hoffentlich...
1. Nutze die Initialisierungsliste, wenn möglich.
2. Ich sehe wenig Konsistenz bzgl deiner Datentypen. sizeof liefert size_t, GetSize gibt aber int zurück. (mal abgesehen davon, dass, wie ZuK bereits erwähnte, dort noch ein logischer Fehler drin steckt) changeElementAt nimmt als Index ein long, op[] jedoch ein int.
3. Statt PrintAll ist vielleicht eine Überladung von op << sinnvoll. Siehe dazu ein Bsp, wie man das mit std::ostream realisiert.
4. Bau dir asserts für Bereichsprüfungen ein, zB in op [].
5. 'IntVektor operator=(const IntVektor &iv)' schaut seltsam aus. Erstmal wird dort der aktuellen Instanz nichts zugewiesen. Und zweitens ist es sinnvoller, dass der Rückgabewert eine Referenz ist, also 'IntVektor&'.
6. Wozu 'const int bla' als Funktionsparameter? Lass das const weg, es ist nicht notwendig.
7. Evtl brauchst du für op [] noch eine const Variante, praktisch das, was GetElementAt macht. Dann gib aber auch eine const Referenz zurück und keine Kopie ('const int& operator[](std::size_t index) const')
8. Die Strategie hinter index halte ich für unglücklich. In size steht doch, wieviel Speicher überhaupt reserviert ist. Und index (sowas wie length wäre vielleicht ein besserer Name) gibt an, wieviel Elemente tatsächlich im Vektor sind. Dementsprechend würde ich index (length?) auch mit 0 initialisieren, und die Klasse entsprechend anpassen.

also ich hab noch gar kein buch gelesen, nur tutorials im internet (daher mein DANKE an Volkard und Schornboeck..... )

@stimmy: deine klasse sieht doch auch nicht so schlecht aus, ist doch alles i.o oder?

deine klasse sieht doch auch nicht so schlecht aus, ist doch alles i.o oder?

das schon
jedoch wenn ich mir deinen sourcecode anschaue verstehe ich davon etwa 50-60% alles als logisch. insofern muss ich schon sagen das du für drei monate (was bei C++ wenig ist!) und nicht mal ein buch gelesen hast, gute fortschritte machst

groovemaster schrieb:

ZuK schrieb:

auf dem ersten blick kann ich keine gröberen fehler erkennen

Ich schon . OK, weniger Fehler, dafür mehr Unregelmässigkeiten.

@Hoffentlich...
1. Nutze die Initialisierungsliste, wenn möglich.
2. Ich sehe wenig Konsistenz bzgl deiner Datentypen. sizeof liefert size_t, GetSize gibt aber int zurück. (mal abgesehen davon, dass, wie ZuK bereits erwähnte, dort noch ein logischer Fehler drin steckt) changeElementAt nimmt als Index ein long, op[] jedoch ein int.
3. Statt PrintAll ist vielleicht eine Überladung von op << sinnvoll. Siehe dazu ein Bsp, wie man das mit std::ostream realisiert.
4. Bau dir asserts für Bereichsprüfungen ein, zB in op [].
5. 'IntVektor operator=(const IntVektor &iv)' schaut seltsam aus. Erstmal wird dort der aktuellen Instanz nichts zugewiesen. Und zweitens ist es sinnvoller, dass der Rückgabewert eine Referenz ist, also 'IntVektor&'.
6. Wozu 'const int bla' als Funktionsparameter? Lass das const weg, es ist nicht notwendig.
7. Evtl brauchst du für op [] noch eine const Variante, praktisch das, was GetElementAt macht. Dann gib aber auch eine const Referenz zurück und keine Kopie ('const int& operator[](std::size_t index) const')
8. Die Strategie hinter index halte ich für unglücklich. In size steht doch, wieviel Speicher überhaupt reserviert ist. Und index (sowas wie length wäre vielleicht ein besserer Name) gibt an, wieviel Elemente tatsächlich im Vektor sind. Dementsprechend würde ich index (length?) auch mit 0 initialisieren, und die Klasse entsprechend anpassen.

so zu 1: ich habe gelesen initlisten nur da nehmen, wo sachen VOR dem cstor rumpf initialisiert werden müssen, das müssen sie hier nciht.
zu 2: mit GetSize() war ich noch nciht fertig, sinn dahinter ist, das ich die größe (also anfangsgröße 10) das array hat, unabhängig davon, wieviele elemente ich eingetragen habe
zu 3:ja ich habe den dann auch noch überladen, nur ist bei mir so drin, am anfang eine printmethode zu schreiben
zu 4:asserts kapier ich noch nciht so ganz
zu 5:kannst mir mal zeigen wie du das meinst? wenn ich IntVektor& schreibe, bekomm ich fehlermeldung (wie muss das return aussehen??)
zu 6:ich dachte, man sollte überall da wo man nix verändern sollte const schreiben...hm...
zu 7:kannst mir auch das mal erklären?
zu 8:hmm...also index habe ich nur genommen,um das oberste arrayelement zu ermitteln....ich fand das eigentlich als gute lösung

trotzdem danke erstmal

pli

Hoffentlich... schrieb:

pli schrieb:
int GetElementAt(unsigned int pos)const{return array[pos<size?pos:size-1];}
kann ich das auch noch schoener machen?

Schoener:

int GetElementAt(unsigned int pos) const
{
  if( pos < size )
  {
    return array[pos];
  }
  else
  {
    // if pos out of bounds return last entry, maybe display a warning
    // std::cout << "WARNING: Requested position out of bounds. Returning last position.\n";
    return array[index];
  }
}

Ok, blaeht den Quelltext etwas auf, aber ist deutlich sicherer und jeder der mehr als 3 Zeilen programmiert hat schon mindestens einmal wegen einem Segmentation Fault gekotzt der wegen nem falschen Index kam

groovemaster

Hoffentlich... schrieb:

so zu 1: ich habe gelesen initlisten nur da nehmen, wo sachen VOR dem cstor rumpf initialisiert werden müssen, das müssen sie hier nciht.

Müssen nicht, ist aber effektiver. Nehmen wir mal diesen ctor

IntVektor(){size=10;array=new int[size];index=-1;}

und erstellen eine Instanz mit static Lebensdauer. Was passiert nun? Zuerst werden alle Member mit 0 initialisiert. Dann werden deine Zuweisungen durchgeführt.

size  <- 0
array <- Nullzeiger
index <- 0
size  <- 10
array <- new[size] Zeiger
index <- -1

Wenn du Glück und einen guten Compiler hast, werden überflüssige Sachen rausoptimiert. Verlassen kann man sich darauf aber nicht.
Nun mit Initialisierungsliste

IntVektor()
    : size(10)
    , array(new int[size])
    , index(-1)
{
}

Alle Member werden sofort mit den gewünschten Werten initialisiert ohne vorherige zero-initialization.
Natürlich kann eine Initialisierungsliste nicht immer so einfach benutzt werden. Was zB, wenn man eine nothrow Variante von new benutzt? Dann musst du auf einen Nullzeiger prüfen.
Zu beachten ist noch, dass die Elemente in der Initialisierungsliste nicht nach ihrer Anordnung dort, sondern nach der Anordnung in der Klassendefinition initialisiert werden. Auf den ersten Blick mag deshalb der ctor korrekt aussehen, wenn in der Klassendefinition aber folgendes steht

int* array;
int size;
int index;

dann hast du undefiniertes Verhalten, weil bereits size ints an Speicher angefordert werden, obwohl size erst später initialisiert wird.

Hoffentlich... schrieb:

zu 4:asserts kapier ich noch nciht so ganz

Asserts sind einfach nur Bedingungen, die erfüllt sein müssen. Man verwendet sie, um logische Programmfehler zu finden. Dh Fehler, die es eigentlich gar nicht geben darf. Ansonsten hat der Programmierer irgendwas falsch gemacht. Ist die Bedingungen nicht erfüllt, wird das Programm irgendeinen Fehler signalisieren. Asserts werden normalerweise im Release Build rausoptimiert, wodurch kein Laufzeitoverhead entsteht. Im Debug Build bleiben sie jedoch erhalten, wodurch man Fehler aufspüren und fixen kann. Bsp

int& operator [](int index_)
{
    assert(index_ >= 0 && index_ < index);
    return array[index_];
}

Wenn du deine Klasse in gewissen Punkten auf unsigned Typen umstellst (was imo empfehlenswert ist), dann verkürzt sich die Bedingung entsprechend.

Hoffentlich... schrieb:

zu 5:kannst mir mal zeigen wie du das meinst? wenn ich IntVektor& schreibe, bekomm ich fehlermeldung (wie muss das return aussehen??)

Ich vermute mal der Fehler/die Warnung ist irgendwas mit "Referenz auf ein lokales Objekt zurückzugeben ist ungültig" öä
op = kann zB so aussehen

IntVektor& operator=(const IntVektor &iv)
{
    // hier machst du im Grunde das, was du bereits im copy ctor machst
    return *this;
}

Hoffentlich... schrieb:

zu 6:ich dachte, man sollte überall da wo man nix verändern sollte const schreiben...hm...

Schon richtig, nur hast du eine lokale Kopie des übergebenen Wertes. Dh, den usprünglichen Wert kannst du sowieso nicht ändern. Das const bewirkt lediglich, dass du die lokale Kopie nicht ändern kannst. Das macht aber in den wenigsten Fällen Sinn.

Hoffentlich... schrieb:

zu 7:kannst mir auch das mal erklären?

Was genau verstehst du denn nicht?

Hoffentlich... schrieb:

zu 8:hmm...also index habe ich nur genommen,um das oberste arrayelement zu ermitteln....ich fand das eigentlich als gute lösung

Kannst du letztendlich machen wie du willst. Nur ist es nicht unbedingt die naheliegendste Lösung und macht die ganze Sache an einigen Stellen imo etwas "dirty".

net 0

Harry Hirsch schrieb:

net schrieb:

...und bei 'Grow' immer verdoppeln ist vielleicht auch nicht geschickt

Doch. Genau das ist geschickt. Ein linearers Wachstum wäre ungeschickt. Siehe dazu mal das Wachstumsverhalten von std::vector.
Bei linearem Wachstum hast Du sonst eine amortisierte lineare Laufzeit beim Hinzufügen von Elementen. Bei multiplikativem Wachstum hält man die Laufzeit in etwa konstant.

du hast recht. das sah oberflächlich erstmal doof aus aber nach genaueren hinsehen ist es gar nicht so schlecht

groovemaster schrieb:

IntVektor& operator=(const IntVektor &iv)
{
    // hier machst du im Grunde das, was du bereits im copy ctor machst
    return *this;
}
Hoffentlich... schrieb:

zu 6:ich dachte, man sollte überall da wo man nix verändern sollte const schreiben...hm...

Schon richtig, nur hast du eine lokale Kopie des übergebenen Wertes. Dh, den usprünglichen Wert kannst du sowieso nicht ändern. Das const bewirkt lediglich, dass du die lokale Kopie nicht ändern kannst. Das macht aber in den wenigsten Fällen Sinn.

Hoffentlich... schrieb:

zu 7:kannst mir auch das mal erklären?

Was genau verstehst du denn nicht?

Hoffentlich... schrieb:

zu 8:hmm...also index habe ich nur genommen,um das oberste arrayelement zu ermitteln....ich fand das eigentlich als gute lösung

Kannst du letztendlich machen wie du willst. Nur ist es nicht unbedingt die naheliegendste Lösung und macht die ganze Sache an einigen Stellen imo etwas "dirty".

Also ist mein Zuweisungsoperator so erstmal ok?
Zu 8:wie genau würdest du es denn machen, wenn nicht mit so einem Index?

groovemaster

Hoffentlich... schrieb:

Zu 8:wie genau würdest du es denn machen, wenn nicht mit so einem Index?

Wie ich bereits sagte, ich würde eine Membervariable length nehmen. Diese gibt an, wieviel Elemente bereits im Vektor sind. Der Wertebereich von length wäre dementsprechend 0 <= length <= size. Du kannst aber auch index nehmen, wenn dir das sympathischer ist. Nur würde ich dann index nicht auf das letzte Element zeigen lassen, sondern 1 Element dahinter, sozusagen auf das nächste Element. Praktisch gesehen, läuft das Handling dann auf das gleiche wie mit length hinaus.

Caipi

@Hoffentlich...
Im Prinzip macht der Zuweisungsoperator genau das gleiche wie der Kopier-Konstruktor. Jedoch musst du dir im klaren darüber sein, dass du beim Zuweisungsoperator (der wars ;)) vorher noch den Speicher freigeben solltest. Da du ihn ansonsten unbrauchbar machst.

Also ich würde dann den Zuweisungsoperator etwa so gestalten:

IntVektor& operator=(const IntVektor &iv)
{
    if(this != &iv) // Keine Selbstzuweisung
    {
         size = iv.size;
         index = iv.index;
         delete[] array; // Bevor du neuen Speicher allokierst erstmal freigeben
         array = new int[size];

         for(int i=0;i<size;i++)
              array[i]=f.array[i]; 
    }
    return *this;
}

Caipi