Lesbarer Quellcode ?

Artchi

Die Funktion CopySupplier ist viel zu lang. Außerdem würde ich sowas auf 100 Zeichen formatieren. Ma muß den Kopf beim Lesen von gaaanz links nach gaaanz rechts bewegen, um eine Zeile zu lesen. Wäre das in der Tageszeitung auch so, würde man bekloppt werden.

Die letzte for-Schleife könnte man idealerweise in eine eigene private Methode extrahieren. Haste kein Refactoringtool? Zwei Mouseklicks und fertig. Ich schreibe meistens auch so lange Funktionen, aber sobald ich die fertig habe, ist erstmal Refactoring angesagt, damit ich vernünftig weiter arbeiten kann.

Den Abschnitt "Lieferanten kopieren" kann man auch in eine eigene Methode extrahieren. Überall wo solche Blockkomentare sind, kann man potenziell extrahieren.

SciFi

Artchi schrieb:

Die letzte for-Schleife könnte man idealerweise in eine eigene private Methode extrahieren. Haste kein Refactoringtool? Zwei Mouseklicks und fertig. Ich schreibe meistens auch so lange Funktionen, aber sobald ich die fertig habe, ist erstmal Refactoring angesagt, damit ich vernünftig weiter arbeiten kann.

Daran habe ich auch schon gedacht, aber diese Funktion ist eine Memberfunktion des Lieferantenstammes und ich brauche diese nie wieder. Ich könnte das ganze jetzt in 2 bis 3 private Methoden aufteilen, aber eine Copy Methode schien mir sinnvoller. Ein Refactoring Tool besitze ich leider nicht. Werde ich mal nach googlen.

Es ist ist aber nicht so, dass der Code unter aller Sau ist oder ? Es steckt immer viel Arbeit drin, gerade in der GUI, welche ich eh nicht gerne mache.

Marc++us

Doch.

Du machst den beliebten Fehler, wie man ihn bei GUIlastigen Programmen immer wieder sieht, Du mischst GUI-Logik, Applikationslogik und Treiberebene in einem einzigen Funktionsaufruf.

Artchi

SciFi schrieb:

Daran habe ich auch schon gedacht, aber diese Funktion ist eine Memberfunktion des Lieferantenstammes und ich brauche diese nie wieder.

Das ist überhaupt kein Argument, sorry. Alles was private ist, wird potenziell nirgends woanders gebraucht. Vielleicht innerhalt einer Klasse, klar. Aber darum geht es ja nicht. Wenn die for-Schleife ausgelagert wird, liest sich der Code besser. Und der Fehler lässt sich einfacher lokalisieren, wenn ein Fehler auftreten sollte. Du hast da einen Kommentar hingeschrieben:
//Gruppen durchlaufen und aendern

Warum machst du daraus nicht eine private Funktion manipulateGroups() oder sowas? 1. könnteste dir den Kommentar sparen, und 2. wäre die Schleife eine eigene Einheit. Durch letzteren Punkt kann man schon mehr Fehler vermeiden, auch in Zukunft.

SciFi schrieb:

Es ist ist aber nicht so, dass der Code unter aller Sau ist oder ? Es steckt immer viel Arbeit drin, gerade in der GUI, welche ich eh nicht gerne mache.

Yo, überhaupt ganz schlecht, das du fachliche Logic in der GUI stehen hast. Sowas gehört ausgelagert, in eine eigene Schicht. Wenn nicht sogar in eine eigene Library (ist dann aber der extremste, aber auch beste, Fall).

Kommt auch darauf an, ob wxWidgets überhaupt sogute Konzepte anbietet? Wenn nicht, muß man sich sowas suchen oder selber designen.

SciFi

Eigentlich sind die Sachen getrennt. Die Lieferanten, Gruppen, Artikel, Belege, Position etc sind alle in einer einen Library (Core). Normalerweise wird hier alles erledigt. Die GUI greift nur noch auf die fertigen Klassen zu.

In diesem Fall war es aber anders. Die Kopierfunktion habe ich eigentlich ganz bewusst mit in die GUI aufgenommen. Hier werden auch nur die Klassen geladen, resettet und neu gespeichert.

Das mein Code nun unter aller Sau ist schockt mich schon, aber das Leben geht weiter.

Artchi

Hihi, aber du wolltest es ja wissen. :p

Aber du wirst dich die nächsten Jahre verbessern. Ist bei jedem so.

Michael E.

Unter aller Sau würde ich das nicht bezeichnen. Es gibt deutlich schlimmeres, glaub mir

rüdiger

[quote="SciFi"]

const bool CSupplierCopyGUI::CopySupplier(void)

ok, das const bool als Rückgabetyp sinnlos ist hast du ja selbst erkannt

Allgemein ist die Funktion viel zu lang. Teile Aufgaben lieber in kleine und wohl definierte (== verständlicher Name) Unterfunktionen auf.

CSupplier Supplier(m_lpCore, m_sSource.c_str(), true);

C als Klassen-Prefix ist Oberpfui. (Ok, hört sich so an, als könntest du nicht viel dafür). Auch die API des Konstruktors scheint nicht ideal zu sein, wenn du auf einmal mit c-Strings rumhantieren musst.

CMySQLQuery *lpGroupQuery = new CMySQLQuery(m_lpCore->GetCMySQL());

Nimm für so etwas lieber einen Smart-Pointer, da du sonst Exception-Sicherheit kaum gewährleisten kannst (ich empfehle dir mal ein bisschen in GotW zu lesen.

Ansonsten finde ich dein Namensschema uneindeutig. Mal fangen Variablen mit großen Buchstaben an, manchmal mit kleinen, wobei du dann noch unverstänliche Buchstabenfolgen an den Anfang hängst (gut, ich weiß das soll diese sinnlose UN sein. Aber mal ehrlich. GroupQuery kannst du dir merken und wenn du dir merken kannst, das es lpGroupQuery heißt, dann kannst du dir auch merken das es ein Pointer ist...)

//Gruppen durchlaufen und aendern
	for (int nGroupIndex = 0; nGroupIndex != lpGroupQuery->GetResultCount(); nGroupIndex++)

das sollte vermutlich kein int sein, sondern eher was in Richtung unsigned ...

SciFi

rüdiger schrieb:

C als Klassen-Prefix ist Oberpfui.

Ehrlich ? Ich hatte das mal am Anfang so gelernt und seitdem nicht mehr abgelegt.

rüdiger schrieb:

(Ok, hört sich so an, als könntest du nicht viel dafür). Auch die API des Konstruktors scheint nicht ideal zu sein, wenn du auf einmal mit c-Strings rumhantieren musst.

Das ist auf meinen Mist gewachsen. Als ich den Kern geplant habe, wusste ich noch nicht, welche GUI ich nehmen soll. Da jetzt jede Lib Ihre eigenen String hat (QString, wxString) etc und ich den Kern unabhängig haben wollte (reines C++ mit MySQL) habe ich mich für die const char * Variante entschieden. Intern arbeitet die Klasse mit strings.

rüdiger schrieb:

CMySQLQuery *lpGroupQuery = new CMySQLQuery(m_lpCore->GetCMySQL());
Nimm für so etwas lieber einen Smart-Pointer, da du sonst Exception-Sicherheit kaum gewährleisten kannst (ich empfehle dir mal ein bisschen in GotW zu lesen.

Das weiss ich genau was ich mache. Der Zeiger ist gültig, bis sich das Programm beendet. Der Kern (Core) hat die MySQL Klasse als Member.

rüdiger schrieb:

Ansonsten finde ich dein Namensschema uneindeutig. Mal fangen Variablen mit großen Buchstaben an, manchmal mit kleinen, wobei du dann noch unverstänliche Buchstabenfolgen an den Anfang hängst (gut, ich weiß das soll diese sinnlose UN sein. Aber mal ehrlich. GroupQuery kannst du dir merken und wenn du dir merken kannst, das es lpGroupQuery heißt, dann kannst du dir auch merken das es ein Pointer ist...)

Normalerweise fangen bei mir Variablen immer klein an. Und zwar mit der UN Bezeichnung : nIndex etc. Die einzige Ausnahme bilden Klassen. Die fangen immer groß und ohne präfix an.

Das mache ich auch schon seitdem ich C++ programmiere. Früher habe ich mit VB geabeitet und kreuz und quer gearbeitet. Als ich auf C++ umgestiegen bin, wollte ich alles richtig machen UN etc. Nun ist alles Humbug

rüdiger schrieb:

//Gruppen durchlaufen und aendern
	for (int nGroupIndex = 0; nGroupIndex != lpGroupQuery->GetResultCount(); nGroupIndex++)
das sollte vermutlich kein int sein, sondern eher was in Richtung unsigned ...

Da gebe ich Dir recht.

So, nun bin ich genug geprügelt worden

Marc++us

SciFi schrieb:

Das weiss ich genau was ich mache. Der Zeiger ist gültig, bis sich das Programm beendet. Der Kern (Core) hat die MySQL Klasse als Member.

Das meint er doch gar nicht.

Die Frage ist, wer lpGroupQuery freigibt, falls die SQL-Abfrage eine Exception wirft, was ja immer mal vorkommen kann.

Btw, ich finde viel schlimmer als die Namenskonvention weiterhin, daß Applikationslogik und Treiberlogik in die GUI gemischt wurde.

SciFi

Marc++us schrieb:

SciFi schrieb:

Das weiss ich genau was ich mache. Der Zeiger ist gültig, bis sich das Programm beendet. Der Kern (Core) hat die MySQL Klasse als Member.

Das meint er doch gar nicht.

Die Frage ist, wer lpGroupQuery freigibt, falls die SQL-Abfrage eine Exception wirft, was ja immer mal vorkommen kann.

Btw, ich finde viel schlimmer als die Namenskonvention weiterhin, daß Applikationslogik und Treiberlogik in die GUI gemischt wurde.

Die Klasse wirft keine Execptions. Die ist auch von mir. Ich arbeite mit Rückgabecodes.

rüdiger

Das mit dem C am Anfang von Klassennamen hat Microsoft damals für die MFC eingeführt, weil es noch keine Namespaces gab. Damit sollten Namenskonflikte verhindert werden. Aber nun sind dummerweise einige Leute dazu übergegangen das C selbst zu verwenden (aus welchem Grund auch immer, der Lesbarkeit bringt es definitiv nichts oder ist es interessant ob es sich um eine Klasse, Struct, Typedef oder was auch immer handelt?). Das ist nicht sehr sinnvoll.

Ich finde es auch nicht unbedingt sinnvoll Objekte anders zu behandeln als PODs oder Variablen von Builtin-Typen (zB in der Namensgebung). Das zeugt imho eher von einer zumindest antiquierten Verwendung der Objektorientierung.

UN ist wirklich humbug. Das führt zu folgenden Problemen: Du denkst zu viel über den Typ nach, dabei ist eine Variable ja eher ein abstraktes Konzept von etwas. Welcher Typ sich genau dahinter verbirgt ist egal, solange er dem Konzept entspricht (also eine Variable index kann zB vom Typ unsigned, std::size_t, std::vector<T>::size_type, Index etc sein. Das ist im Grunde eher uninteressant).

Außerdem verkompliziert es das programmieren. Du musst wahrscheinlich regelmäßig nachschauen wie die Variable genau heißt (fängt er nun mit einem l oder i oder n etc an?). Solchen Namen kann man sich schlecht merken. Da schaut man lieber mal in dem Fall genau nach, in dem man den Typ genau wissen muss.

SciFi schrieb:

Marc++us schrieb:

SciFi schrieb:

Das weiss ich genau was ich mache. Der Zeiger ist gültig, bis sich das Programm beendet. Der Kern (Core) hat die MySQL Klasse als Member.

Das meint er doch gar nicht.

Die Frage ist, wer lpGroupQuery freigibt, falls die SQL-Abfrage eine Exception wirft, was ja immer mal vorkommen kann.

Btw, ich finde viel schlimmer als die Namenskonvention weiterhin, daß Applikationslogik und Treiberlogik in die GUI gemischt wurde.

Die Klasse wirft keine Execptions. Die ist auch von mir. Ich arbeite mit Rückgabecodes.

Selbst wenn alle Methoden/Funktionen die du aufrufst nie nie nie im Leben eine Exception werfen würden (was ich nicht glaube), enthält dein Code sogar eine Stelle an der eine Exception fliegen kann.

Es ist nicht leicht Exception sicheren Code zu schreiben!

matimatiker

Rückgabecodes statt Exceptions sind böse.
Rückgabecodes kann man ignorieren, tut man auch => unauffindbare Fehler entstehen.

Was diese ungarische notation angeht. Was haltet ihr davon diese nicht zu verwenden um den typen zu "dokumentieren", sondern den Verwendungszweck der Variable. Z.b. int bIstAn ? bIstAn kann 0 oder 1 sein, bzw. true oder false?

rüdiger

matimatiker schrieb:

Was haltet ihr davon diese nicht zu verwenden um den typen zu "dokumentieren", sondern den Verwendungszweck der Variable. Z.b. int bIstAn ? bIstAn kann 0 oder 1 sein, bzw. true oder false?

Wo ist der Sinn davon? Wenn man eine Variable einschränken will, dann nimmt man eben einen entsprechenden Typ (in dem Fall zB bool) oder bastelt eine entsprechende Klasse. Dafür gibt es ja OOP

matimatiker schrieb:

Rückgabecodes statt Exceptions sind böse.
Rückgabecodes kann man ignorieren, tut man auch => unauffindbare Fehler entstehen.

Exceptions kann man in C++ aber auch ignorieren, was das ganze arbeiten damit komplizierter macht.

matimatiker

naja ist vorschrift für mich auf der arbeit. habe anfangs auch nicht den sinn davon verstanden. Diese Beispiele sind halt sinnvoller:

int* pInt;
SmartPtr<int> pInt;
int* bufInt;

und kommt mir nicht damit für bufInt std::vector vorzuschlagen. Um z.b. binary-dateien einzulesen ist es nutzlos.

matimatiker

naja schrieb:

matimatiker schrieb:

Rückgabecodes statt Exceptions sind böse.
Rückgabecodes kann man ignorieren, tut man auch => unauffindbare Fehler entstehen.

Exceptions kann man in C++ aber auch ignorieren, was das ganze arbeiten damit komplizierter macht.

Ja, aber um die zu ignorieren musst du aufwand betreiben. um rückgabekodes zu ignorien muss du keinen. im gegenteil um auf rückgabecodes zu testen musst du aufwand betreiben.

worauf ich hinaus will?
faulheit

Edit:
Nagut manchmal überwieder der aufwand eine fehler zu beseitigen den aufwand die entsprechende exception zu ignorieren...

vista

matimatiker schrieb:

Ja, aber um die zu ignorieren musst du aufwand betreiben. um rückgabekodes zu ignorien muss du keinen.

gute compiler geben ein warning aus, wenn man return values ignoriert...

matimatiker

vista schrieb:

matimatiker schrieb:

Ja, aber um die zu ignorieren musst du aufwand betreiben. um rückgabekodes zu ignorien muss du keinen.

gute compiler geben ein warning aus, wenn man return values ignoriert...

nicht alle compiler sind gut. es ist imho nicht sinnvol die warnung standardmäßig auszugeben. vielleicht ist es ja absicht dass ich einen rückgabewert ignoriere.
Anders gesagt der Aufwand ist immer noch da:
* guten compiler finden.
* entsprechende compiler-flag in der doku suchen
* compiler-flag anfügen

vista

matimatiker schrieb:

vielleicht ist es ja absicht dass ich einen rückgabewert ignoriere.

dann musst du die funktion so aufrufen

(void)funktion_die_was_zurueckgibt();