Überprüfung von objektorientiertem Code

[Firefighter]

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

[Nexus]

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.

[asc]

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é

[Gast]

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

[-.]

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?

[drakon]

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.

[drakon]

-. 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.

[Gast]

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

[drakon]

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.

[Gast]

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

[drakon]

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.

[Gast]

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.

[unskilled]

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

[Bashar]

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.

[unskilled]

Bashar schrieb:

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.

das halte ich für falsch...

bool tu_ichs_oder_nicht (true);
tu_ichs_oder_nicht ? object->function ("ausdruck"):object->other_function ("ausdruck");

[queer_boy]

was willst du uns sagen? dass da nur ausdrücke (expressions) stehen dürfen? dann ergibt aber nichts mehr wirklich sinn

[Gast]

unskilled schrieb:

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

Es soll anzeigen, ob es schwarz oder weiß ist. In meinem Code ist das auch ein bool. Das Problem war, dass ich die abgeleitete Klasse noch erweitern wollte, mir sind aber nur die Eigenschaften Farbe und witzig eingefallen.

[Mehlvogel]

Unregistrierter schrieb:

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.

Gerade diese Denkweise wird dir schnell zum Kreuz. Vom OO Design Standpunkt (und um den ging es dir ja u.a. auch im ersten Posting) sollten die Klassen die Daten halten nichts mit den Klassen zu tun haben, die diese Daten ausgeben. Das sorgt dafür, dass auch 3 Monate später, wenn man seine Bilbiothek zu Übungszwecken mal in eine GUI Anwendung packen will, nicht den ganzen Code umschreiben muss, der nur für die Daten zuständig ist, sondern diesen einfach wiederverwenden kann - ein Kernaspekt von gutem OO Design.

[Gast]

Aber wenn ich den ostream Operator überlade, kann ich das doch
auch nicht in eine GUI Anwendung packen, da es ja iostream ist.

[drakon]

Unregistrierter schrieb:

Aber wenn ich den ostream Operator überlade, kann ich das doch
auch nicht in eine GUI Anwendung packen, da es ja iostream ist.

iostream gehört zur Standardbibliothek und ist somit eigentlich überall verfügbar.