Lesbarer Quellcode ?
-
Diese Funktion ist in der GUI. Der Supplier (CSupplier) ist eine Klasse aus dem Kern. Das Reset bedeutet, dass die Zuordnung in der Datenbank aufgehoben wird und mit dem Save wird dieser Datensatz neu geschrieben. Der Displaytext ist der Anzeigename des Lieferanten in der Übersicht. Diese wird in eine Treeview geladen.
& marc++us
Der m_wxTextDestination ist ein wxTextCtrl Object. Dieses Textfeld beinhaltet die Beschreibung (Displaytext) des neuen Lieferanten. Dieses Feld muss ich mit GetValue() auslesen.
Danke für eure Kritik
-
SciFi schrieb:
Der m_wxTextDestination ist ein wxTextCtrl Object. Dieses Textfeld beinhaltet die Beschreibung (Displaytext) des neuen Lieferanten. Dieses Feld muss ich mit GetValue() auslesen.
Das ist mir klar. Aber wozu hast Du den Kommentar geschrieben?
-
Marcus meinte auch nicht der Code wäre falsch sondern dass der Kommentar genau null Mehrwert hat. Wäre genau so was wie hier
// Jetzt lassen wir den Zähler i laufen for (int i = ...) ...
-
Du hast Recht. "Zielbeschreibung" ist etwas unglücklich. Ich meinte die Beschreibung für den Ziellieferanten.
-
CengizS schrieb:
Marcus meinte auch nicht der Code wäre falsch sondern dass der Kommentar genau null Mehrwert hat. Wäre genau so was wie hier
// Jetzt lassen wir den Zähler i laufen for (int i = ...) ...Sowas habe ich hier schon mal gesehen. Im ernst. Man, ich lag unterm Tisch vor Lachen. Ging garnicht.
-
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.
-
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.
-
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.
-
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 aendernWarum 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.
-
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.
-
Hihi, aber du wolltest es ja wissen. :p
Aber du wirst dich die nächsten Jahre verbessern. Ist bei jedem so.
-
Unter aller Sau würde ich das nicht bezeichnen. Es gibt deutlich schlimmeres, glaub mir
-
[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 ...
-
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
-
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.
-
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.
-
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!
-
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?
-
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.