3. Überprüfung von objektorientiertem Code

Überprüfung von objektorientiertem Code

  • ?

    Hey 🙂
    Ich bin mit meinem C++ Buch fertig geworden, und wollte jetzt
    erstmal OOP richtig üben.
    Ein Freund gab mir den Tipp, eine Bibliothek zu programmieren.
    Das ist tatsächlich eine gute Übung. Könntet ihr bitte mal in den Code
    reinschaun, und sagen, was man noch verbessern könnte ( Performance, Übersicht usw. ) oder wie ich die
    Bibliothek noch erweitern könnte?
    Der Code besteht aus einer Basisklasse Buch und einer
    abgeleiteten Klasse Comic. Hier mal der Code...

    #include <iostream>
#include <string>

using std::cout;
using std::endl;
using std::cin;using std::string;

class Buch
{
private:
   string titel;   
   string autor;
   unsigned int seiten;
   double preis;
public:
   Buch();
   Buch( string titel, string autor, unsigned int seiten, double preis );
   void printbuch();
   ~Buch();
   void set_titel( string titel );
   void set_autor( string autor );
   void set_seiten( unsigned seiten );
   void set_preis( double preis );
   string get_titel();
   string get_autor();
   unsigned get_seiten();
   double get_preis();
};

class Comic : Buch
{
private:
   bool farbe;
   bool witzig;
public:
   Comic();
   Comic( string titel, string autor, unsigned int seiten, double preis, bool farbe, bool witzig );
   void printcomic();
   ~Comic();
   void testfarbe( bool farbe );
   void testwitzig( bool witzig );
};

int main() 
{
   Buch buch1( "C++ von A bis Z", "Juergen Wolf", 1000, 39.99 );
   buch1.printbuch();
   Buch buch2;
   buch2.printbuch();
   Comic comic1( "Goofy", "Goofy2", 53, 10.33, 1, 1 );
   comic1.printcomic();
   Comic comic2;
   comic2.printcomic();
   cin.get();
   return 0;
}

Buch::Buch() 
{
   titel = "Unbekannt";
   autor = "Unbekannt";
   seiten = 0;
   preis = 0;
}

Buch::Buch( string titel, string autor, unsigned int seiten, double preis )
{
   this->titel = titel;
   this->autor = autor;
   this->seiten = seiten;
   this->preis = preis;
}

void Buch::printbuch()
{
   cout << "Titel: " << titel << endl;
   cout << "Autor: " << autor << endl;
   cout << "Seiten: " << seiten << endl;
   cout << "Preis: " << preis << endl;
   cout << endl;
}

Buch::~Buch()
{
   cout << titel << " wurde aus der Bibliothek geschmissen!" << endl;
}

void Buch::set_titel( string titel ) 
{
   this->titel = titel;
}

void Buch::set_autor( string autor )
{
   this->autor = autor;
}

void Buch::set_seiten( unsigned seiten )
{
   this->seiten = seiten;
}
void Buch::set_preis( double preis )
{
   this->preis = preis;
}

string Buch::get_titel()
{
   return titel;
}

string Buch::get_autor()
{
   return autor;
}

unsigned Buch::get_seiten()
{
   return seiten;
}

double Buch::get_preis()
{
   return preis;
}

Comic::Comic()
{
   set_titel( "Unbekannt" );
   set_autor( "Unbekannt" );
   set_seiten( 0 );
   set_preis( 0 );
   farbe = 0;
   witzig = 0;
}

Comic::Comic( string titel, string autor, unsigned int seiten, double preis, bool farbe, bool witzig )
{
   set_titel( titel );
   set_autor( autor );
   set_seiten( seiten );
   set_preis( preis );
   this->farbe = farbe;
   this->witzig = witzig;
}

void Comic::printcomic()
{
   cout << "Titel: " << get_titel() << endl;
   cout << "Autor: " << get_autor() << endl;
   cout << "Seiten: " << get_seiten() << endl;
   cout << "Preis: " << get_preis() << endl;
   cout << "Farbe: "; this->testfarbe(farbe);
   cout << endl;
   cout << "Witzig: "; this->testwitzig(witzig);
   cout << endl << endl;
}

Comic::~Comic()
{
   cout << get_titel() << " wurde aus der Bibliothek verschmissen! " << endl;
}

void Comic::testfarbe( bool farbe ) // 1 = Farbe
{
   if( farbe )
      cout << "vorhanden"; 
   else if( !farbe )
      cout << "nicht vorhanden";
}

void Comic::testwitzig( bool witzig ) // 1 = witzig
{
   if( witzig )
      cout << "witzig"; 
   else if( !farbe )
      cout << "nicht witzig";
}

    Sagt einfach, was euch auffällt, egal welche Art von Fehlern ihr entdeckt.
    Wäre sehr nett. Liebe Grüße

    0
  • D

    Ich glaube er hat "Bibliothek" nicht so wörtlich gemeint. 😉

    Aber man kann auch so eine Bibliothek schreiben. 🙂

    - in ne lib gehört (im Header) keine using!
    - initialisierungsliten (nachschlagen)
    - getter/setter inline

    So Sachen:

    void Comic::testwitzig( bool witzig ) // 1 = witzig
{
   if( witzig )
      cout << "witzig";
   else if( !farbe )
      cout << "nicht witzig";
}

    mach besser:

    bool Comic::is_witzig ()const{return witzig;}

    Hmm. Ansonsten wirst du, denke ich vieles anderst machen,wenn du ein wenig mehr Übung hast,aber mach nur mal ein grösseres Projekt, dann kommt das besser, wenn du merkst, was du anderst machen solltest, oder was hald nicht so gut geht, als das wir dir "vorschreiben". 😉

    0
  • E

    Habe ich es übersehen? Sehe keinen Copy-Constructor und Assign-Operator in der Klasse.

    0
  • D

    Erhard Henkes schrieb:

    Habe ich es übersehen? Sehe keinen Copy-Constructor und Assign-Operator in der Klasse.

    Ist ja auch nicht nötig.

    Wäre aber ein Thema, dass er (wahrscheinlich) doch auch nochmal genauer anschauen sollte.

    0
  • E

    Da wird es doch erst spannend. 😉

    0
  • ?

    Erstmal Danke für die Antworten.

    drakon schrieb:

    Ich glaube er hat "Bibliothek" nicht so wörtlich gemeint. 😉

    Aber man kann auch so eine Bibliothek schreiben. 🙂

    - in ne lib gehört (im Header) keine using!
    - initialisierungsliten (nachschlagen)
    - getter/setter inline

    So Sachen:

    void Comic::testwitzig( bool witzig ) // 1 = witzig
{
   if( witzig )
      cout << "witzig";
   else if( !farbe )
      cout << "nicht witzig";
}

    mach besser:

    bool Comic::is_witzig ()const{return witzig;}

    Hmm. Ansonsten wirst du, denke ich vieles anderst machen,wenn du ein wenig mehr Übung hast,aber mach nur mal ein grösseres Projekt, dann kommt das besser, wenn du merkst, was du anderst machen solltest, oder was hald nicht so gut geht, als das wir dir "vorschreiben". 😉

    Also mit Bibliothek meine
    ich natürlich nicht eine Programmbibliothek oder so etwas in der Art,
    sondern der Code an sich soll eine Bibliothek darstellen. 😃

    Dieser Code

    void Comic::testwitzig( bool witzig ) // 1 = witzig
{
   if( witzig )
      cout << "witzig";
   else if( !farbe )
      cout << "nicht witzig";
}

    Ist ja dazu gedacht, auszugeben ob es jetzt witzig ist oder nicht. Den rufe ich
    bei der Ausgabe von einem Comic auf.

    bool Comic::is_witzig ()const{return witzig;}

    Gibt ja nur witzig zurück.

    Wozu dient das const in diesem Fall?
    Einen Kopierkonstruktor werde ich noch einfügen.

    0
  • F

    Dieses

    const

    dient in dem Fall dazu das explizit gesagt wird "Diese Methode hat keinerlei Rechte Member der Klasse zu ändern" solltest du trotzdem mit der Methode versuchen einen Member zu ändern, wird dich dein Compiler höfflich darauf hinweisen dass, das was du probieren willst nicht erlaubt ist 🙂

    0
  • N

    Bei diesem Code musst du aufpassen, denn wenn der Fall eintrifft, dass witzig falsch und farbe wahr ist, wird nichts ausgegeben. Ist das deine Absicht? Wenn ja, würde ich einen passenderen Bezeichner suchen oder zwei einzelne Funktionen machen, weil die Farbe nicht unbedingt mit der Witzigkeit zusammenhängt.

    void Comic::testwitzig( bool witzig ) // 1 = witzig 
{ 
   if( witzig ) 
      cout << "witzig"; 
   else if( !farbe ) 
      cout << "nicht witzig"; 
}

    Ansonsten würde ich die Bezeichner vereinheitlichen. Also entweder du schreibst immer set_titel , test_witzig (wobei ich hier auch zu is_witzig raten würde) - also immer mit Unterstrich zwischen den Wörterteilen, oder gleich SetTitel , IsWitzig , GetSeiten .

    Und dass du beim Konstruktor eine Initialisierungsliste verwenden solltest, wurde schon gesagt. Im Weiteren würde ich die Parameter nicht gleich wie die Member nennen, da du so schnell den Überblick verlierst und immer this-> verwenden musst. Mach doch z.B. die Member wie bisher (z.B. autor ) und die Parameter werden dann entsprechend gekennzeichnet (z.B. new_autor ).

    Was ich mir bei Vererbung auch überlegen würde, wäre, die privaten Member protected zu machen, dann kannst du von der abgeleiteten Klasse auch ohne Schnittstellen auf die Member der Basisklasse zugreifen.

    Und noch zum const : Grundsätzlich kannst du es überall dort verwenden, wo du nichts am Objekt änderst. Also beispielsweise bei den Getter-Methoden.

    0
  • A

    Unregistrierter schrieb:

    Dieser Code

    void Comic::testwitzig( bool witzig ) // 1 = witzig
{
   if( witzig )
      cout << "witzig";
   else if( !farbe )
      cout << "nicht witzig";
}

    Ist ja dazu gedacht, auszugeben ob es jetzt witzig ist oder nicht. Den rufe ich
    bei der Ausgabe von einem Comic auf.

    Dies ist kein guter Ansatz. Du bindest dich damit zu sehr an die Oberfläche (Hier Konsole). Was ist wenn du mal auf eine UI wechselst? Willst du dann die Klasse Comic jedesmal ändern?

    Was hat ein "Comic" mit der Ausgabe zu tun? Richtig: Nichts.

    cu André

    0
  • ?

    Nexus schrieb:

    Bei diesem Code musst du aufpassen, denn wenn der Fall eintrifft, dass witzig falsch und farbe wahr ist, wird nichts ausgegeben. Ist das deine Absicht? Wenn ja, würde ich einen passenderen Bezeichner suchen oder zwei einzelne Funktionen machen, weil die Farbe nicht unbedingt mit der Witzigkeit zusammenhängt.

    void Comic::testwitzig( bool witzig ) // 1 = witzig 
{ 
   if( witzig ) 
      cout << "witzig"; 
   else if( !farbe ) 
      cout << "nicht witzig"; 
}

    Ansonsten würde ich die Bezeichner vereinheitlichen. Also entweder du schreibst immer set_titel , test_witzig (wobei ich hier auch zu is_witzig raten würde) - also immer mit Unterstrich zwischen den Wörterteilen, oder gleich SetTitel , IsWitzig , GetSeiten .

    Und dass du beim Konstruktor eine Initialisierungsliste verwenden solltest, wurde schon gesagt. Im Weiteren würde ich die Parameter nicht gleich wie die Member nennen, da du so schnell den Überblick verlierst und immer this-> verwenden musst. Mach doch z.B. die Member wie bisher (z.B. autor ) und die Parameter werden dann entsprechend gekennzeichnet (z.B. new_autor ).

    Was ich mir bei Vererbung auch überlegen würde, wäre, die privaten Member protected zu machen, dann kannst du von der abgeleiteten Klasse auch ohne Schnittstellen auf die Member der Basisklasse zugreifen.

    Und noch zum const : Grundsätzlich kannst du es überall dort verwenden, wo du nichts am Objekt änderst. Also beispielsweise bei den Getter-Methoden.

    Ups, jetzt fällt es mir erst auf. Es muss natürlich

    else if( !witzig)

    heißen, für Farbe habe ich ja eine eigene Methode gebaut.

    Danke für die guten Tipps, werde ich mir merken 🙂

    0
  • ?

    drakon schrieb:

    - getter/setter inline

    Kann eine Funktion denn tatsächlich für "nicht-friend-zugreifende" inline sein wenn in ihr auf private members zugegriffen wird?

    0
  • D

    Unregistrierter schrieb:

    Ups, jetzt fällt es mir erst auf. Es muss natürlich

    else if( !witzig)

    heißen, für Farbe habe ich ja eine eigene Methode gebaut.

    Danke für die guten Tipps, werde ich mir merken 🙂

    Das dachte ich mir, dass da was falsches passiert. 😉

    Wobei auch diese überprüfung unsinnig wäre. Da kannst du doch einfach ein else machen.
    Was aber auch noch nicht gut ist, wie asc bereits gesagt hat, wenn du jetzt eine Bibliothek schreibst (allgmein, nicht unbeding eine für Bücher), dann darf so etwas da gar nicht rein. Die Ausgabe hat ja nichts damit zu tun, ob es jetzt witzig ist, oder nicht. Du kannst natürlich den << - Operator überladen und dann eine Funktion schreiben, die dir alles auf einen std::ostream ausgibt, dann kannst du das nämlich wirklich einfach in eine Datei oder auf die Konsole ausgeben.

    Zum Beispiel so:

    ostream& Buch::operator << (std::ostream& os, Buch const& buch)
{
    if(buch.witzig)
        os << "witzig";
...
}

    Schau dir dazu einfach Operatorenüberladung an und wegen dem std::ostream musst du dich hald ein wenig mit der Standardbibliothek anfreunden. 😉

    0
  • D

    -. schrieb:

    drakon schrieb:

    - getter/setter inline

    Kann eine Funktion denn tatsächlich für "nicht-friend-zugreifende" inline sein wenn in ihr auf private members zugegriffen wird?

    inline hat nichts mit dem Datenzugriff zu tun.

    0
  • ?

    drakon schrieb:

    Unregistrierter schrieb:

    Ups, jetzt fällt es mir erst auf. Es muss natürlich

    else if( !witzig)

    heißen, für Farbe habe ich ja eine eigene Methode gebaut.

    Danke für die guten Tipps, werde ich mir merken 🙂

    Das dachte ich mir, dass da was falsches passiert. 😉

    Wobei auch diese überprüfung unsinnig wäre. Da kannst du doch einfach ein else machen.
    Was aber auch noch nicht gut ist, wie asc bereits gesagt hat, wenn du jetzt eine Bibliothek schreibst (allgmein, nicht unbeding eine für Bücher), dann darf so etwas da gar nicht rein. Die Ausgabe hat ja nichts damit zu tun, ob es jetzt witzig ist, oder nicht. Du kannst natürlich den << - Operator überladen und dann eine Funktion schreiben, die dir alles auf einen std::ostream ausgibt, dann kannst du das nämlich wirklich einfach in eine Datei oder auf die Konsole ausgeben.

    Zum Beispiel so:

    ostream& Buch::operator << (std::ostream& os, Buch const& buch)
{
    if(buch.witzig)
        os << "witzig";
...
}

    Schau dir dazu einfach Operatorenüberladung an und wegen dem std::ostream musst du dich hald ein wenig mit der Standardbibliothek anfreunden. 😉

    Das verstehe ich nicht ganz. Wenn ich einen Comic erstelle, dann kann ich ja festlegen, ob er witzig ist (1 = Witzig).
    Ist doch egal, ob ich dazu eine Funktion schreibe,
    die den Bool in eine Ausgabe umwandelt, oder ob ich dafür den
    ostream Operator überlade 😕
    Und warum lege ich mich damit auf die Console fest?
    Die anderen Funktionen benutzen auch cout 😕

    else if( !witzig)

    Stimmt, da hätte ein else gereicht, danke für den Tipp 🙂

    0
  • D

    Unregistrierter schrieb:

    Das verstehe ich nicht ganz. Wenn ich einen Comic erstelle, dann kann ich ja festlegen, ob er witzig ist (1 = Witzig).
    Ist doch egal, ob ich dazu eine Funktion schreibe,
    die den Bool in eine Ausgabe umwandelt, oder ob ich dafür den
    ostream Operator überlade 😕
    Und warum lege ich mich damit auf die Console fest?
    Die anderen Funktionen benutzen auch cout 😕

    Also du kannst dann so etwas schreiben:

    Buch b;
std::cout << "Das von ihnen gewaehlte Buch hat folgende Beschreibung: " << b;

std::ofstream out ("buch.txt");
out << b;

    Du solltest dann die Informatinonen in der Funktion so formatieren, wie du es haben willst. (Natürlich nicht nur ob es witzig ist, sonder auch noch Name, Titel usw.)
    Und das tolle ist,dass du das gleiche auch mit Files (oder anderen ostreams) machen kannst, ohne dir zu überlegen, wie du das jetzt (jedesmal) genau formatieren willst.

    0
  • ?
     

    if( witzig )
      cout << "witzig";
   else if( !witzig )
      cout << "nicht witzig";

    Aber um das kommt man bei deiner Methode ja auch nicht drum herum?
    Ich erkläre nochmal genau was ich damit bezwecke.
    Also mit dieser Methode gebe ich ja den Comic aus...

    void Comic::printcomic()
{
   cout << "Titel: " << get_titel() << endl;
   cout << "Autor: " << get_autor() << endl;
   cout << "Seiten: " << get_seiten() << endl;
   cout << "Preis: " << get_preis() << endl;
   cout << "Farbe: "; this->testfarbe(farbe);
   cout << endl;
   cout << "Witzig: "; this->testwitzig(witzig);
   cout << endl << endl;
}

    Damit will ich alle Eigenschaften ausgeben, also auch ob es Farben hat und ob es witzig ist. Aber ich kann ja nicht einfach schreiben

    cout << farbe

    bzw

    cout << witzig

    denn da farbe und witzig bools sind, würde das 0 oder 1 ausgeben. Darum habe ich diese Methode geschrieben, die dann witzig ausgibt, wenn witzig "eingeschaltet" ist. Wenn ich << überlade, dann würde das doch auch 0 oder 1 ausgeben, wenn man diese Methode weglässt. Sorry, ich stehe irgendwie auf der Leitung

    0
  • D

    Klar geht das, aber es tut nicht das, was manche erwarten würden. 😉

    Das hier wohl schon eher:

    class buch
{
public:
	buch(bool w, int f, std::string n, int s, float p):
		witzig(w),
		  farbe (f),
		  name(n),
		  seiten(s),
		  preis(p){}

		//Beachte hier, dass du du die values bekommst. get sollte nichts ausgen, sondern nur
		//etwas ZURÜCKGEBEN. Also den erwarteten Wert.
		std::string get_titel () const {return name;}
		float		get_preis () const {return preis;}
		//..
private:
	bool witzig;
	int farbe;
	std::string name;
	int seiten;
	float preis;
	//..

	friend std::ostream& operator << (std::ostream& os, buch const& b);
};

std::ostream& operator << (std::ostream& os, buch const& b)
{
	b.witzig?os << "witzig\n":os << "nicht witzig\n" ;
	os << "Farbe: " << b.farbe << "\n";
	os << "Name: " << b.name << "\n";
	os << "Seiten: " << b.seiten << "\n";
	os << "Preis: " << b.preis << "\n";
	//..
	return os;
}

    buch c (true, 5, "hmm",10000,0.99f);
std::cout << "Ihr gewaehltes Buch: \n" << c;

    btw:

    value?anweisung1:anweisung2;

    Nennt sich ternärer Operator. Wenn value wahr ist, mache anweisung1, sonst anweisung2.

    0
  • ?

    jo stimmt, den << Operator zu überladen, wäre hier schon sinnvoll.
    Aber da ich die Bücher sowieso nur auf der Console ausgeben will, rentiert es sich nicht, den Code zu ändern.

    0
  • U

    drakon schrieb:

    btw:
    Zitat:

    value?anweisung1:anweisung2;

    Nennt sich ternärer Operator. Wenn value wahr ist, mache anweisung1, sonst anweisung2.

    und ist immer wieder herrlich unübersichtlich 😛

    b.witzig?os << "witzig\n":os << "nicht witzig\n" ;

    vs.

    if (b.witzig)
   os << "witzig\n";
else os << "nicht witzig\n";

    oder:

    if (!b.witzig)
   os << "nicht ";
os << "witzig\n";

    aber ich würd wo das normale if bevorzugen ^^

    bb

    edit:

    ach so - warum int farbe?
    willst du die anzahl verwendeten farben speichern?
    so, wie ich die klasse ma verstanden (überflogen) hatte, sollte das heißen, ob sie schwarz/weiß sind oder in farbe... aber das is ja eher nebensächlich

    0
  • B

    drakon schrieb:

    value?anweisung1:anweisung2;

    Nennt sich ternärer Operator. Wenn value wahr ist, mache anweisung1, sonst anweisung2.

    Nö, Anweisungen dürfen da nicht stehen, nur Ausdrücke.

    0
Anmelden zum Antworten