Wieviele LOC für Funktion?

objekt orientiert

Die Schreib Weise ist auch *brr*

volkard

Neuling1 schrieb:

ich würde gerne wissen wie stark ihr versucht eure Funktionen klein zu halten. Mir hat jemand mal gesagt dass wenn eine Funktion größer als 10 Zeilen bzw. nicht mehr auf einen Blick auf den Bildschirm passt dann ist etwas faul bzw. schlecht designed.

der hat an sich recht.

Aber ich kann mir nicht vorstellen dass es effizient ist straightforward alle 10 Zeilen einen Funktionsaufruf zu machen nur um was kleines (Variablen, sonstiges) zu setzen. Das schlägt sich doch auch irgendwo auf die Performance nieder ständig variablen auf den stack zu schmeißen etc...

nö, gar nicht. eher im gegenteil. die funktionen kriegen ja auch nicht hunderte von parametern, sondern ganz wenige, die sollen ja nur einen zweck erfüllen und keine multi-mach-drei-grundverschiedene-sachen-monster sein. also ich als speed-junky, der gerne mal eine funktion mikrootimiert, bevor er die anwendung durchplant, muß genausohübsch zerlegen wie Shade, der das bestimmt aus vernünftigeren erwägungen macht.

habe da mal einen bericht gelesen, wo das übliche monolithische malloc aufgespalten wurde, um die verschiedenen unterziele jeweils mit verschiedenen mthoden implementieren zu können und dann ganz viele kombinationebn ausprobieren zu können. malloc ist ja immer ein dickes monster, um teure aufrufe zu sparen. am ende war das zerlegte malloc einfach so schon schneller als das dicke monster und ich mußte mir erstmal die lachtränen aus den augen wischen, bevor ich zum lesen der vermutungen über bessere cachetreffer und sprungvorhersage kam.

Oder ist das schon korrekt so? Wie macht ihr das? Wie groß sind eure Methoden?

wo es kompliziert wird, gehen meine funktionen eigentlich immer auf aus 6 oder weniger zeilen runter. größer wirds mal, wenn ich in bäumen rumlaufe oder wenn ich dicke rechenausdrücke in trivialrechnungen zerlege. aber die funktionen fangen nicht mit 6 zeilen an, sondern fallen im laufe des optimierens erst zusammen.
ein klassiker ist das stack::push. mal probieren, ob ichs auswendig hinkriege

void push(T const& t){
   Node* newNode=new Node;
   newNode->next=anchor;
   newNode->date=t;
   anchor=newNode;
}

hmm. an sich ok. geht auch praktisch nicht schneller. oder doch ja, an einer stelle. hier wird in new Node erstmal das Node::data leer angelegt und nachher zugewiesen, statt es direkt reinzukonstruieren. hier würde die verwendung eines Node-Konstruktors mit initialisiererliste speed bringen.
also

void push(T const& t){
   Node* newNode=new Node(anhor,t);
   anchor=newNode;
}

uups, jetzt bettelt die funktion aber drum, daß ich weitermache

void push(T const& t){
   anchor=new Node(anhor,t);
}

das passiert einfach von allein.

natürlich gibt es sachen, die nicht zerlegt werden.

out<<"<html>";
out<<"<head>";
...//noch 50 zeilen, wo nur gelegentlich eine funktion aufgerufen wird, zum beispiel writeLinkToRootFrom(this)
out<<"</html>";

das zerlege ich dann nicht in head und body oder gar den body noch in teile.

Xebov

Shade Of Mine schrieb:

Komplettes redesign.

Du kannst in C++ objekt orientiert programmieren, also tu das auch.

Also ich finde das ganze schon Objektorienteirt. Es gibt ne Modelklasse die verschiedene Member hat, dazu gehört ein Array aus ModelParts (ein struct) das für jeden Modelpart die Kollisionsboxen (Sphere, MinBox usw) und den Vertex/Indexbuffer usw hält. Die Modelklasse bekommt den Auftrag ein Model aus ner Datei zu laden (der Code war wohlgemerkt nur ein kleienr Teil, als Funktion am Stück wären alle 5 Funktionen zum laden mehrere Seiten lang) und stopft diese dann in die entsprechenden member in die sie gehören.

Shade Of Mine schrieb:

zB Code wie

BYTE *VertexTmp=new BYTE[m_pModelParts[i].dVertexSize*m_pModelParts[i].iVertices];
            fread_s(VertexTmp,m_pModelParts[i].dVertexSize*m_pModelParts[i].iVertices,m_pModelParts[i].dVertexSize,m_pModelParts[i].iVertices,Model);
                    //Vertexliste übertragen in den Buffer
            m_pModelParts[i].VertexBuffer.AddVertices(m_pModelParts[i].iVertices,VertexTmp);
                        //Vertexbuffer Updaten
            m_pModelParts[i].VertexBuffer.Update();
            delete[] VertexTmp;

ist *brr*

Wieso? Das Exceptions fehlen und der Speicher nicht gesichert ist weiß ich, die Klasse steht ja auch auf eienr Überarbeitungsliste, aber sonst wo ist da das Problem? Hier wird soviel Speicher angelegt das alle Vertices komplett reinpassen und der Bereich wird dann an den Vertexbuffer zum eintragen durchgereicht.

Shade Of Mine schrieb:

[unterer rest]

Naja von der Idee alles auszulagern bin ich nicht sehr begeistert. Dein Vorschlag würde bedeuten ich lese bis zur passenden Stelle und reiche die Datei dann eine Runde herum so das jeder Teil sich seinen Anteil selebr rausholt, gerade wenn sich das Format mal ändert finde ich das irgendwie nicht so ansprechend.

Beim Umschreiben mir C++ Sprachmitteln (was ich später noch mache) würde es am Ende fast genauso aussehen, nur das es halt C++ Sprachmittel sind. Aber mal eine Frage, wieso fidnest du es Sinnvoll beim Auslesen aus eienr datei die Datei herumzureichen statt sie Zentral auszulesen und das Ergebniss weiterzugeben?

Shade Of Mine

Xebov schrieb:

Also ich finde das ganze schon Objektorienteirt. Es gibt ne Modelklasse die verschiedene Member hat, dazu gehört ein Array aus ModelParts (ein struct) das für jeden Modelpart die Kollisionsboxen (Sphere, MinBox usw) und den Vertex/Indexbuffer usw hält.

Und jetzt zaehle die Substantive und die Klassen.
Die Daumen*PI Regel ist ja, jedes Substantiv ist eine moegliche Klasse.

Wieso? Das Exceptions fehlen und der Speicher nicht gesichert ist weiß ich, die Klasse steht ja auch auf eienr Überarbeitungsliste, aber sonst wo ist da das Problem? Hier wird soviel Speicher angelegt das alle Vertices komplett reinpassen und der Bereich wird dann an den Vertexbuffer zum eintragen durchgereicht.

Ja, es muss ueberarbeitet werden, da sind wir uns ja einig.
Wenn du es richtig OO machen wuerdest, wuerdest du zB den unnoetigen Temporaeren Speicher sogar sparen koennen...

Naja von der Idee alles auszulagern bin ich nicht sehr begeistert. Dein Vorschlag würde bedeuten ich lese bis zur passenden Stelle und reiche die Datei dann eine Runde herum so das jeder Teil sich seinen Anteil selebr rausholt, gerade wenn sich das Format mal ändert finde ich das irgendwie nicht so ansprechend.

Gerade eine Aenderung der Struktur ist dann ja trivial.
Du rufst in readModelParts() ja nur
readBoundingBox()
readVertices()
readMatrix()
readSphere()
etc.

auf. Wenn sich jetzt das jetzt aendert, ist es trivial die passende Stelle zu finden und anzupassen. Auch Unittests sind so viel sinnvoller moeglich.

Was wenn du zB die Daten ge-gzipt einlesen willst? Fuer sowas hast du dann ja den iterator auf den Speicherbereich der es dir entzippen kann. Oder du supportest ein 2. Format das grundlegende Aehnlichkeiten mit dem aktuellen hat: du kannst problemlos bestehende funktionen verwenden.

uU hast du nur eine XML Datei die den Aufbau des jeweiligen Formats beschreibt. etc. Alles kein problem - da du viele kleine entitaeten hast und eine anpassung einer entitaet ist trivial.

das problem mit so langen funktionen ist, dass in 2-3 Jahren niemand sich mehr traut die funktion anzufassen. denn um sie zu aendern muss ich sie komplett verstehen. eine 2-10 Zeilen Funktion kann man dagegen viel einfacher verstehen und anpassen.

zB auch die Kommentare die du geschrieben hast: die sparst du dir auch dadurch dass du diese abschnitte in funktionen packst...

eine Frage, wieso fidnest du es Sinnvoll beim Auslesen aus eienr datei die Datei herumzureichen statt sie Zentral auszulesen und das Ergebniss weiterzugeben?

Ich wuerde einen iterator weiterreichen.
Warum muss es eine Datei sein? Kannst du die Daten nicht aus einer Datenbank lesen? Oder per Memory Mapping in den Speicher laden? uU laedst du die Daten von einem Server herunter oder sie ist verschluesselt, etc.

Das ist eben der OO-Ansatz. Kleine Entitaeten. Code wird nicht OO durch OO-Sprachmittel. Code wird durch das Design OO.

knivil

Du kannst in C++ objekt orientiert programmieren, also tu das auch.

Not every problem is an object. Du kannst in C++ auch imperativ/prozedural programmieren, also tue das auch. Und Teile eines Format- / Dateikonverters werden wohl selten von anderen Konvertern benutzt oder wiederverwendet. Da macht eine Modularisierung wenig Sinn.

Die Daumen*PI Regel ist ja, jedes Substantiv ist eine moegliche Klasse.

Eine Java-Philosophie :). Dazu ein etwas kritischer Artikel: execution-in-kingdom-of-nouns

Zum Thema: Funktionen sind bei mir so lang, wie sie sinnvoll sind. Was sinnvoll ist, das sagt mir meine Erfahrung. Erfahrung braucht Zeit und kommt auch mit der Zeit.

Shade Of Mine

knivil schrieb:

Eine Java-Philosophie :). Dazu ein etwas kritischer Artikel: execution-in-kingdom-of-nouns

Sehe den Zusammenhang nicht. In Java packt man alles in Klassen weil man nicht anders kann. Es ist nunmal so, dass ein Nomen ein hinweis auf eine Klasse ist. Natuerlich muss man auch so weit gehen und sagen ein verb ist ein Hinweis auf eine Funktion. Und ein Adjektiv ist ein Hinweis auf ein Property. Aber darum geht es hier ja nicht. Wenn ich eine struct erstelle, dann kann ich daraus gleich was ordentliches machen mit Ctor. Und das ist der Punkt.

Structs die nur Daten enthalten habe ich eigentlich nie - weil es keinen Vorteil bringt. Wenn ich eine struct habe die Daten enthaelt, kann ich gleich einen Ctor und gaengige Funktionen zur Manipulation von ihnen anbieten.

Bestes Beispiel ist hier wohl volkards Stack Beispiel. Wenn man entitaeten in sich schliesst, dann kuerzt man Code enorm (und vorallem: man macht ihn robuster).

Xebovs Code wuerde zB sogar einen enormen Geschwindigkeitsvorteil bekommen...

Bashar

volkard schrieb:

ein klassiker ist das stack::push.

Das ist aber unlauterer Wettbewerb. Ein Stack ist etwas generisches, außerdem das Standardbeispiel in unzähligen Büchern und Tutorials. Eine einzigartige Applikationslogik in gleicher Weise auseinanderzustrahieren ist ungleich schwerer, der Nutzen nicht unmittelbar einsichtig, manchmal ist es sogar kontraproduktiv.

pumuckl

Shade Of Mine schrieb:

Xebov schrieb:

Viel Spaß beim auseinandernehmen, bin echt mala uf das Ergebniss gespannt.

Komplettes redesign.

Seh ich genauso. Man könnte auch erwidern: Viel Spaß beim Debuggen. Viel Spaß beim Pflegen. Viel Spaß beim Verstehen wenn der Code nach 6 Monaten wieder angefasst werden muss.

Badestrand

Bashar schrieb:

Eine einzigartige Applikationslogik in gleicher Weise auseinanderzustrahieren ist ungleich schwerer, der Nutzen nicht unmittelbar einsichtig, manchmal ist es sogar kontraproduktiv.

pumuckl

Bashar schrieb:

Eine einzigartige Applikationslogik in gleicher Weise auseinanderzustrahieren ist ungleich schwerer,

Ist eine Behauptung. Gegegenbehauptung: Ist nur dann schwerer wenn die Logik in schlechtem Design umgesetzt wird.

der Nutzen nicht unmittelbar einsichtig,

Wie stehts mit Lesbarkeit, Wartbarkeit, Verständlichkeit, Erweiterbarkeit? Ist das nicht Nutzen genug?

manchmal ist es sogar kontraproduktiv.

Definiere kontraproduktiv in dem Zusammenhang und zeig dann wo es kontraproduktiv sein soll. Ich stimme nur dann zu, wenn kontraproduktiv = "kurzfristig zeitaufwändiger (und langfristig interessiert nicht)"

volkard

manchmal. und vor allem sind die gewinne nicht unmittelbar ersichtlich, weshalb mans besser trotzdem tut.

Xebov

Shade Of Mine schrieb:

Xebovs Code wuerde zB sogar einen enormen Geschwindigkeitsvorteil bekommen...

Wieso würde der Code nen Geschwindigkeitsvorteil bekommen? Was spricht den gegen ein Struct zum lagern der Daten? Was hätte ich davon wenn ne Modelklasse die daten in ein Array legt und diese daten in enr eigenen klasse sind und somit alles was rein muß zu den datens elbst durch muß statt vond er Klasse direkt behandelt zu werden, das wäre ja doppelt gemoppelt.

Bashar

pumuckl schrieb:

Bashar schrieb:

Eine einzigartige Applikationslogik in gleicher Weise auseinanderzustrahieren ist ungleich schwerer,

Ist eine Behauptung. Gegegenbehauptung: Ist nur dann schwerer wenn die Logik in schlechtem Design umgesetzt wird.

Ich verstehe deine Gegenbehauptung nicht. Ein gutes Design zu finden ist nur schwer, wenn man es schlecht macht?

der Nutzen nicht unmittelbar einsichtig,

Wie stehts mit Lesbarkeit, Wartbarkeit, Verständlichkeit, Erweiterbarkeit? Ist das nicht Nutzen genug?

Doch, aber das bekommst du nicht automatisch. Dazu muss das Design nämlich auch gut sein. Den Code von Xebov kann man einfach von oben nach unten lesen und verstehen. Wenn du das ganze durch ein Klassenkonstrukt ersetzt, wird es erstmal komplizierter, es sei denn, du machst es wirklich richtig gut.

manchmal ist es sogar kontraproduktiv.

Definiere kontraproduktiv in dem Zusammenhang und zeig dann wo es kontraproduktiv sein soll. Ich stimme nur dann zu, wenn kontraproduktiv = "kurzfristig zeitaufwändiger (und langfristig interessiert nicht)"

Kontraproduktiv heißt, dass der Code schwerer verständlich und weniger wartbar wird.

pumuckl

Bashar schrieb:

Ich verstehe deine Gegenbehauptung nicht. Ein gutes Design zu finden ist nur schwer, wenn man es schlecht macht?

Nein. Ein gutes Design ist schwer zu finden wenn schon das übergeordnete Design schlecht ist.

Doch, aber das bekommst du nicht automatisch. Dazu muss das Design nämlich auch gut sein. Den Code von Xebov kann man einfach von oben nach unten lesen und verstehen. Wenn du das ganze durch ein Klassenkonstrukt ersetzt, wird es erstmal komplizierter, es sei denn, du machst es wirklich richtig gut.

Den Code von Xebov find ich alles andere als leicht zu lesen und zu verstehen. Die Funtkion erfüllt mehrere Aufgaben auf einmal, auf verschiedenen Abstraktionsebenen (von byteweise I/O bis zu den ModelParts) und benutzt zu allem Überfluss auch noch schlecht gewählte Namen (wenn ich ModelParts und Model lese, gehe ich davon aus, dass Model ein Modell ist, das aus modelParts besteht - es scheint sich aber um eine Datei zu handeln). Natürlich muss das Design gut sein damit man die Vorteile von sauberem Code nutzen kann. Aber ein gutes Design zu finden ist eben nicht so schwer wie einige behaupten.

Kontraproduktiv heißt, dass der Code schwerer verständlich und weniger wartbar wird.

Noch schwerer verständlich als ein Moloch von über 60 Zeilen wäre nur eine Funktion mit noch mehr Zeilen oder schlechter gewählten Variablennamen. Ich werd mich nachher zu Hause mal kurz dransetzen und versuchen zu skizzieren wie man die Funktion ein wenig lesbarer gestalten könnte.

Xebov

pumuckl schrieb:

Den Code von Xebov find ich alles andere als leicht zu lesen und zu verstehen. Die Funtkion erfüllt mehrere Aufgaben auf einmal, auf verschiedenen Abstraktionsebenen (von byteweise I/O bis zu den ModelParts) und benutzt zu allem Überfluss auch noch schlecht gewählte Namen (wenn ich ModelParts und Model lese, gehe ich davon aus, dass Model ein Modell ist, das aus modelParts besteht - es scheint sich aber um eine Datei zu handeln). Natürlich muss das Design gut sein damit man die Vorteile von sauberem Code nutzen kann. Aber ein gutes Design zu finden ist eben nicht so schwer wie einige behaupten.

Das leseproblem liegt nur daran das du die Klassenvariablen nicht kennst. Die Funktion ist von oben nach unten zu lesen, sie liest nur Daten ein und reicht sie ggf an die Richtige Stelle weiter, ich sehe da keine Stellen wo die Funktion nochetwas anderes tut als einlesen und weitergeben. Die Funktion liest aus einer Modelldatei die Modell-Teile ein. Deswegen heist die Datei auch Model. Leichte rweiterbar ist die Funktion auch, ich hab für den Dateitypen eine Kleine Dokumentation geschrieben in der steht was an welcher Stelle steht und in welchem Format.

Um dir noch etwas Überblick zu verschaffen wenn du dann selbst was Skizieren willst. Die Funktion ist Teil eienr Gruppe von Einlesefunktionen, die Klasse öffnet eine Modell-Datei als Model und fängt an einzulesen (das Dateidesign habe ich so gemacht das man sie ohne springen am Stück von oben nahc unten lesen kann) Sie liest aus der datei die Anzahl der Model-Teile aus (ModelParts) und erstellt ein Array eienr Struktur in richtiger Größe (ModelParts) dieses Struktur beinhaltet alle Details über den Modelteil, und diese Details werden in dieser Funktion nacheinander für jeden ModelPart eingelesen (die liegen auch in der Datei hintereinander)

volkard

pumuckl schrieb:

Den Code von Xebov find ich alles andere als leicht zu lesen und zu verstehen.

naja, echt schrecklich ist was anderes. der code geht so. und ist unreparierbar, außer man schmeißt verdammt viel weg. außerdem ist bei gamecoderz manches suboptimal, deaswegen liest sich der code auch ganz angenehm, weil man diese funktion und ihre vettern schon lange kennt. und wenn man's anders macht, entfernt man sich von der gemeinde, was beim gegenseitig-helfen wieder von nachteil ist.

unskilled

Das leseproblem liegt nur daran das du die Klassenvariablen nicht kennst

super - damit ist er wohl wartbar, wenn man erst alle klassenvariablen kennen muss, um den code zu verstehen? Oo

die Funktion nochetwas anderes tut als einlesen und weitergeben

also erfüllt sie nur eine aufgabe - oder doch mehrere?!

klar macht es ihn nicht zu furchtbar schlechtem code, den niemals mehr wieder irgendwer verstehen kann - aber es macht ihn nicht zu perfektem Code - und das man ihn übersichtlicher gestalten könnte, solltest du nun auch langsam mal glauben ^^

bb

Nexus

volkard schrieb:

und wenn man's anders macht, entfernt man sich von der gemeinde, was beim gegenseitig-helfen wieder von nachteil ist.

Das ist mal eine Legitimation.

Man kann übrigens auch beim Spiele-Programmieren versuchen, aufs Design zu achten. Bei dieser Art von Programmen, die unter Umständen besonders bug- und wartungsanfällig ist, kann sich das erst recht auszahlen. Manchmal ist sauberes Design eben nicht ganz einfach, besonders, wenn man faul ist und schnelle Resultate erzielen möchte. Es braucht relativ viel Erfahrung, bis man merkt, welche Strukturierung einem Vorteile in der Handhabung bringt und sich auch längerfristig bewährt...

unskilled schrieb:

aber es macht ihn nicht zu perfektem Code

Irgendwann kommt der Punkt der Entscheidung zwischen Aufwand für Design und Zweckmässigkeit der Applikation. Am Design kann man oft ewig weiterfeilen, bei komplexeren Problemen wird es immer mehrere Lösungen geben, und so richtig gefällt einem meist doch keine, egal, wie lange man schon daran gearbeitet hat.

Bashar

pumuckl schrieb:

Bashar schrieb:

Ich verstehe deine Gegenbehauptung nicht. Ein gutes Design zu finden ist nur schwer, wenn man es schlecht macht?

Nein. Ein gutes Design ist schwer zu finden wenn schon das übergeordnete Design schlecht ist.

Letzteres kann man in jedem größeren Projekt als gegeben annehmen, daher q.e.d.

Den Code von Xebov find ich alles andere als leicht zu lesen und zu verstehen. Die Funtkion erfüllt mehrere Aufgaben auf einmal, auf verschiedenen Abstraktionsebenen (von byteweise I/O bis zu den ModelParts) und benutzt zu allem Überfluss auch noch schlecht gewählte Namen (wenn ich ModelParts und Model lese, gehe ich davon aus, dass Model ein Modell ist, das aus modelParts besteht - es scheint sich aber um eine Datei zu handeln).

Die gewählten Namen könnten ein Problem sein, allerdings kein Designproblem. Dass die Funktion mehrere Aufgaben auf einmal erfüllt, ist nur dann ein Problem, wenn man diese Teilfunktionalitäten auch separat nutzen möchte. Dann muss man nämlich Code duplizieren. Ob das hier der Fall ist? Aber Wiederverwendbarkeit und Verständlichkeit sind zwei verschiedene Paar Schuh.
Verschiedene Abstraktionsebenen sind es, weil die Funktion die Schnittstelle zwischen zwei solchen bildet, sie konstruiert ja High-Level-Objekte aus Bytes.

Noch schwerer verständlich als ein Moloch von über 60 Zeilen wäre nur eine Funktion mit noch mehr Zeilen oder schlechter gewählten Variablennamen. Ich werd mich nachher zu Hause mal kurz dransetzen und versuchen zu skizzieren wie man die Funktion ein wenig lesbarer gestalten könnte.

Können wir die Variablennamen mal beiseite lassen? Mir geht es um Design, also etwas strukturelles. Und "lesbar" ist auch dehnbar. Mich interessieren konkrete Designprobleme.

Xebov

unskilled schrieb:

super - damit ist er wohl wartbar, wenn man erst alle klassenvariablen kennen muss, um den code zu verstehen? Oo

Nein man muß nicht alle kennen, aber du kannst ja nun auch keinen motor Reparieren wenn du nur 4 Schrauben davon kennst oder?

unskilled schrieb:

also erfüllt sie nur eine aufgabe - oder doch mehrere?!

Eine, Daten einlesen und an einigen Stellen werden sie weitergegeben wenn sie nicht direkt verwertbar sind.