3. Wieviele LOC für Funktion?

Wieviele LOC für Funktion?

  • U

    ok - du hast recht...

    der rest ist dumm, hat keine ahnung oder naja.. ist einfach im unrecht...

    hf

    0
  • X

    unskilled schrieb:

    ok - du hast recht...

    der rest ist dumm, hat keine ahnung oder naja.. ist einfach im unrecht...

    hf

    Bist du jetzt beleidigt weil ich bei eienr Klassenfunktion vorraussetze das man dir Klassenvariablen zum Teil kennt und weil ich es nunmal als eine Aufgabe ansehe die Daten einzulesen und weiterzugeben?

    0
  • S

    Xebov schrieb:

    Wieso würde der Code nen Geschwindigkeitsvorteil bekommen?

    Weil du dir den temporaeren Speicher sparen kannst den du dauernd per new anforderst. Je nachdem wieviel Daten du da hin und her schaufelst kann das relevante Vorteile bringen es einzusparen.

    Was spricht den gegen ein Struct zum lagern der Daten?

    Nichts, nur dass man dann eben auch noch einen Ctor definiert und uU named ctors um die Daten leichter initialisieren zu koennen.

    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.

    Eine struct ist eine Klasse. Einer struct einen ctor und uU sinnvolle Methoden zu geben tut nicht weh. Zumindest der Ctor sollte sein. Da du zB bei deinem Code immer doppelte Initialisierungen hast. Das ist fehleranfaellig. Lieber die Daten gleich direkt initialisieren - das bringt nicht nur mehr lesbarkeit und oft mehr performance, sondern erleichtert auch die Wartbarkeit enorm.

    Wie gesagt, das ganze etwas generischer aufbauen und vorallem von dem rohen lesen der Datei weggehen. Das waere der wichtigste Schritt. Der Rest kommt dann von alleine.

    ob im nachhinein ein redesign sinn macht sei mal dahingestellt. meistens kostet refactoren zuviel zeit. aber wenn moeglich wuerde ich das design so frueh wie moeglich so schoen wie moeglich hinbekommen.

    das bisschen zeit das man dadurch verliert ist irrelevant wenn man dann naemlich spaeter die anforderungen anpassen muss (was bei jedem groesseren projekt sowieso passiert) und dann hat man die zeit wieder drinnen. deshalb: wenn man die zeit hat, so gut designen wie irgend moeglich. gerade am anfang vom projekt ist genug zeit da - die laeuft erst gegen ende aus.

    0
  • T

    Xebov schrieb:

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

    Nö, aber immerhin hat so ein Motor Schrauben. Deiner ist aus einem Stück gegossen und müsste weggeschmissen werden, wenn er kaputt ist.

    0
  • P

    die Funktion nochetwas anderes tut als einlesen und weitergeben

    und temporäre dynamische Arrays verwalten und den Chunk verwalten (wozu auch immer der gut sein soll) und über das ganze Array iterieren...

    Ich bin schon dabei, dauert noch bissl...
    Wird auch noch keinesfalls zufriedenstellend, da ich die äußeren Gegebenheiten nicht kenne.

    0
  • U

    Xebov schrieb:

    Bist du jetzt beleidigt weil ich bei eienr Klassenfunktion vorraussetze das man dir Klassenvariablen zum Teil kennt und weil ich es nunmal als eine Aufgabe ansehe die Daten einzulesen und weiterzugeben?

    ne - aber ist sinnlos, dir erklären zu wollen, dass deine Fkt eben mehr macht, als nur einwas und genau so sinnlos zu erklären, dass deine Fkt weder gut zu lesen (und damit wartbar) noch sonderlich performant ist.

    Das haben dir jz schon mind. 4 versch. Leute gesagt und jedes Mal (gut) begründet - und alles, was du machst ist zu sagen, dass es doch gut ist, so wie du es machst...

    Dazu kommt noch, dass die Fkt in deinen Augen jz super ist, obwohl du vor paar Tagen noch meintest (als die ersten Kritiken kamen), dass du sie ohnehin überarbeiten willst und sie nicht um sonst auf der Liste der zu überarbeiteten Dateien steht...

    bb

    0
  • X

    pumuckl schrieb:

    und temporäre dynamische Arrays verwalten [...] und über das ganze Array iterieren...

    An dieser Stelle kann ich dir leider nicht ganz folgen.

    pumuckl schrieb:

    den Chunk verwalten (wozu auch immer der gut sein soll)

    Das ist ein Datenkopf, der enthält eine Identifikation und die größe der darunterliegenden Daten.

    pumuckl schrieb:

    Ich bin schon dabei, dauert noch bissl...
    Wird auch noch keinesfalls zufriedenstellend, da ich die äußeren Gegebenheiten nicht kenne.

    Macht nichts bin gespannt drauf.

    unskilled schrieb:

    dass deine Fkt eben mehr macht, als nur einwas und genau so sinnlos zu erklären, dass deine Fkt weder gut zu lesen (und damit wartbar) noch sonderlich performant ist.

    Das sie nicht gerade das performanteste Stück Code ist hab ich ja nie angezweifelt. In Punkto Lesbarkeit und Wartbarkeit scheiden sich aber die Geister ganz eindeutig, Bashar zB konnte ihn lesen. Gewartet habe ich ihn selber auch schon, da er von oben nach unten die Datei abbildet lassen sich neue Teile einfach dazwischen einfügen.

    unskilled schrieb:

    Das haben dir jz schon mind. 4 versch. Leute gesagt und jedes Mal (gut) begründet - und alles, was du machst ist zu sagen, dass es doch gut ist, so wie du es machst...

    Das sage ich garnicht, leider erschließt sich mir der Sinn einiger Argumente ganz und garnicht. Ich gebe da mal ein Beispiel. Shade bemängelt den Rohspeicher, er wird geholt, befüllt, an eine andere Klasse durchgereicht und dann wieder gelöscht. Nun ist aber die andere Klasse eine Standalone Vertexbuffer Klasse, die ist nicht allein für den Gebrauch im Model vorgesehen, um den Speicher aber wegzulassen müsste ich die Vertexbuffer Klasse um neue Funktionen erweitern, das hieße aber auch das ich jedesmal wenn ich ein neues Format für die Modelle unterstützen will auch die Vertexbuffer Klasse umbauen müsste obwohl sie eigentlich mit dem Model nichts zu tun hat.

    Und genau hier kann ich der Argumentation nicht folgen weil sich das ganze dnan auf noch mehr Funktionen verteilen würde und man nichtmehr sagen kann das das die Modelklasse für alles zuständig ist.

    unskilled schrieb:

    Dazu kommt noch, dass die Fkt in deinen Augen jz super ist, obwohl du vor paar Tagen noch meintest (als die ersten Kritiken kamen), dass du sie ohnehin überarbeiten willst und sie nicht um sonst auf der Liste der zu überarbeiteten Dateien steht...

    Sie wird ja auch überarbeitet, das ganze File Zeug kommt raus und einige einlese Vorgänge fasse ich zusammen. Aber vom Funktionsinhalt, also dem was sie im großen und ganzen tut sehe ich eben keine Probleme. Den Inhaltlich kann ich sagen die Funktion ließt die Datenblöcke für die Modelteile ein und sendet die eingelesenen Daten an die entsprechenden Datenmember.

    0
  • P

    So, eine erste Überarbeitung meinerseits. Mit einigen Stellen bin ich noch nicht glücklich, aber ich denke man kriegt einen gewissen Eindruck. Obwohl es mir in den Fingern gejuckt hat hab ich drauf verzichtet, Variablennamen zu ändern, z.B. Model -> ModelFile, die in meinen Augen unnötigen m_-Präfixe und die ungarische Notation (die evtl. sogar von einem Framework vorgegeben sein mag).

    Die neuen Methoden für ModelPart sind nicht zwingend erforderlich, runden den einen oder anderen Aufruf in meinen Augen aber ab. Wenn ModelPart vom Framework vorgegeben ist, kann man entweder freie Funktionen definieren die ein ModelPart als Argument nehmen oder von ModelPart ableiten (da es ein struct ist sollte das wenig Probleme bereiten).

    void getModelPart(ModelPart* modelPart);
void bfModel::ReadModelParts()
{
  for (int i=0; i <m_iNumModelParts; ++i)
  {
    getModelPart(m_pModelParts + i);
  }
}

struct ChunkReader //keine Ahnung was das ständige ReadChunk bringen soll, aber naja...
{
  bfChunk chunk;
  ChunkReader() : chunk() 
  { ZeroMemory(&chunk,sizeof(chunk); }

  void read() 
  { ReadChunk(chunk); }

  int getLastiID()
  { return chunk.iID; }
};

template <typename T> void getVar(T& var);
{
  fread_s(&var, sizeof(var), sizeof(var), 1, Model);
}

void getVertices(ModelPart* modelPart);
void getIndices(Modelpart* modelPart);

// Die Hauptfunktion - reicht um zu verstehen was im Groben passiert //
void getModelPart(ModelPart* modelPart)
{
  static ChunkReader chunkReader;
  chunkReader.read();
  chunkreader.read();
  getVar(modelPart->dVertexFormat);
  chunkreader.read();
  getVar(modelPart->iVertices);
  getVar(modelPart->dVertexSize);
  getVar(modelPart->fSphere);
  getVar(modelPart->MinBox);     //nehme an, dass MinBox vom Typ Box ist, sonst separate getBox() Funktion
  getVar(modelPart->MaxBox);
  getVertices(modelPart);
  chunkReader.read();

  getVar(modelPart->iIndices);
  getIndices(modelPart);

  chunkReader.read();
  if (chunkReader.getLastiID() != 0x4140)
  {
    stepBackOneIndex();
  }
  else
  {
    getVar(modelPart->iModelNumber);
    getVar(modelPart->vKoords);  //nehme an, dass vKoords vom eigenen Typ ist, sonst separate getKoords() Funktion
  }
}

void getVertices(ModelPart* modelPart)
{
  size_t bufferSize = modelPart->getVertexBufferSize();
  std::vector<BYTE> buffer(bufferSize);
  fread_s(&buffer[0], bufferSize, modelPart->dVertexSize, modelPart->iVertices, Model);

  modelPart->initVertexBuffer();  //s.u.
  modelPart->addVerticesToVertexBuffer(&buffer[0]);
  modelPart->updateVertexBuffer();
}

void getIndices(Modelpart* modelPart)
{
  size_t bufferSize = modelPart->getIndexBufferSize();
  std::vector<BYTE> buffer(bufferSize);
  fread_s(&buffer[0], bufferSize, 6, modelPart->iIndices, Model);

  modelPart->initIndexBuffer();
  modelPart->addIndicesToIndexBuffer(&buffer[0]);
  modelPart->updateIndexBuffer();

}

//Zusätzliche ModelPart-Methoden

size_t ModelPart::getVertexBufferSize()
{
  return dVertexSize*iVertices;
}

void ModelPart::initVertexBuffer()
{
  VertexBuffer.Init(getVertexBufferSize(), dVertexSize, dVertexFormat);
}

void ModelPart::addVerticesToVertexBuffer(BYTE* source)
{
  VertexBuffer.AddVertices(iVertices, source);
}

void ModelPart::updateVertexBuffer()
{
  VertexBuffer.Update();
}

void ModelPart::initIdexBuffer()
{
  IndexBuffer.Init(iIndices*6, 6);
}

size_t ModelPart::getIndexBufferSize()
{
  return 6*iIndices;
}
void ModelPart::updateIndexBuffer()
{
  IndexBuffer.Update();
}
void ModelPart::addIndicesToIndexBuffer(BYTE* source)
{
  IndexBuffer.AddIndices(iIndices, source);
}

    /edit: eine vergessen:

    void stepBackOneIndex()
{
  fseek(Model,-6,SEEK_CUR);
}
    0
  • X

    Interessante Sache, du hast die Funktion total Zerlegt.

    Ich gebe dir mal noch die Sachen die aus dem ersten ersten Code von mir nicht ersichtlich waren:

    //Das Struct auf dem der ModelPart basiert
struct bfModelPart
{
	DWORD dVertexFormat;
	int iVertices;
	int iIndices;
	DWORD dVertexSize;
	bfIndexBuffer IndexBuffer;
	bfVertexBuffer VertexBuffer;
	bfVector3 MinBox;
	bfVector3 MaxBox;
	float fSphere;
	int iModelNumber;
	bfVector3 vKoords;
};
//Der Chunck Struct
struct bfChunk
{
	unsigned short iID;
	unsigned int iChunkSize;
	int iBytesRead;
};
//Die Relevanten Klassenvariablen
        FILE *Model;
	int m_iNumModelParts;
	bfModelPart *m_pModelParts;//Modeleinzellteile
//Read und SkipChunk
        void ReadChunk(bfChunk &chunk)
	{
		fread_s(&chunk.iID,2,2,1,Model);
		fread_s(&chunk.iChunkSize,4,4,1,Model);
		chunk.iBytesRead=6;
	};
	void SkipChunk(bfChunk &chunk)
	{
		fseek(Model,chunk.iChunkSize-chunk.iBytesRead,SEEK_CUR);
	};

    Was es mit dem ReadChunk auf sich hat, die Datei ist Binär und hat einen Kopf unter dem die Größe steht, ReadChunk liest die nächsten 6Bytes aus. Die Funktion kann dann schauen welcher Teil dort steht, das macht es fürs erweiternd er Datei besodners Leicht weil man die Reihenfolge der Daten nur bedingt beachten muß. Die abfrage siehst du ind er Funktion nicht, aber die Zugehörige Mutterfunktion prüft die Daten, die Unterfunktion tut es (noch) nicht. Das ganze ist im Grunde nur Mittel zur Prüfung.

    0
  • S

    Xebov schrieb:

    Shade bemängelt den Rohspeicher, er wird geholt, befüllt, an eine andere Klasse durchgereicht und dann wieder gelöscht. Nun ist aber die andere Klasse eine Standalone Vertexbuffer Klasse, die ist nicht allein für den Gebrauch im Model vorgesehen, um den Speicher aber wegzulassen müsste ich die Vertexbuffer Klasse um neue Funktionen erweitern, das hieße aber auch das ich jedesmal wenn ich ein neues Format für die Modelle unterstützen will auch die Vertexbuffer Klasse umbauen müsste obwohl sie eigentlich mit dem Model nichts zu tun hat.

    Nein.

    Eine recht simple Moeglichkeit ist zB einen iterator auf die rohen Daten zu verwenden statt die rohe Datei anzufassen. Dann hast du keine interop probleme mehr. Oder aber eine init-Klasse die dir eine vertexbuffer Klasse erstellt (mit move semantik zb).

    Und genau hier kann ich der Argumentation nicht folgen weil sich das ganze dnan auf noch mehr Funktionen verteilen würde und man nichtmehr sagen kann das das die Modelklasse für alles zuständig ist.

    Weil die Modelklasse ja auch nicht fuer alles zustaendig sein soll 😕

    pumuckls Code ist schonmal nicht schlecht und er hat es ohne redesign geschafft - er hat die vorhandenen Funktionen nur aufgebrochen. Macht die Wartung schonmal um Dimensionen einfacher. Und vorallem: du hast ploetzlich keine abhaengigkeit von fread_s mehr.

    Jetzt stell dir mal vor jemand anderer muss den Code warten. Welche Version wuerde ihm wohl besser gefallen? Deine oder pumuckls?

    0
  • P

    Danke für die weiteren Infos, aber ich denk mal als Skizze reicht das - werd mich jetzt wieder meinen eigenen Codes zuwenden 😉

    Ich denk mal dass meine Funktion getModelPart beim ersten Durchlesen schon eher ahnen lässt was vor sich geht als deine bisherige, wo man beim allerersten Lesen hauptsächlich einen großen Haufen ähnliche fread_s-Aufrufe sieht 😉

    0
  • X

    Shade Of Mine schrieb:

    Jetzt stell dir mal vor jemand anderer muss den Code warten. Welche Version wuerde ihm wohl besser gefallen? Deine oder pumuckls?

    Ich finde pumuckls Variante nicht schlecht, wie er ja selbst gesagt hat nicht ganz Perfekt, ich hätte da einige Verbesserungsideen, aber Grundsätzlich finde ichs gut.

    0
  • P

    Wenn ich das richtig verstanden habe und in jedem Chunk anhand der ID abzulesen ist was danach kommt, dann ist ein ModelPart in der Datei ja so aufgebaut:

    chunk mit ID = MODEL_PART
chunk mit ID = MODEL_PART_VERTICES
vertexvariablen
chunk mit ID = MODEL_PART_INDICES
indexvariablen
optional:
  chunk mit ID = MODEL_PART_KOORDS
  modellnumer und koordinaten

    wenn dem so ist würd ichs wie folgt gliedern:

    class ModelReader
{
  bfModel model;  
  bfModelPart modelPart;
  bfChunk chunk;
public:
  bfModel readModelFromFile(std::string modelFileName)
  {
    //unter anderem:
    readAllModelParts();
  }

private:
  void readAllModelParts()
  {
    for (i = 0; i < model.iNumModelParts; ++i)
    {
      checkAndReadModelPart();
      model.modelParts.push_back(modelPart);
    }
  }

  void checkAndReadModelPart()
  {
    checkNextChunkIDEquals(MODEL_PART);
    readModelPart();
  }

  void checkNextChunkIDEquals(unsigned int ID)
  {
    readChunk();
    if (chunk.iID != ID)
      throw UnexPectedChunkException;
  }

  void readModelPart()
  {
    checkAndReadVertices();
    checkAndReadIndices();
    readOptionalKoords();
  }

  cheackAndReadVertices()
  {
    checkNextChunkIDEquals(MODEL_PART_VERTICCES);
    readVertices();
  }

  checkAndReadIndices()
  {
    checkNextChunkIDEquals(MODEL_PART_INDICES);
    readIndices();
  }

  readOptionalKoords()
  {
    if(testNextChunkID(MODEL_PART_KOORDS))
      readKoords();
  }

  bool testNextChunkID(unsigned int ID)
  {
    readChunk();
    if (chunk.iID == ID) return true;
    stepBackOneChunk();
    return false;
  }

  //etc. you get the idea ;)
};

    Wieder nur eine Skizze, die beteiligten Klassen müssten natürlich etwas geändert werden (z.B. einen Container für die Modelparts statt eines dynamischen Arrays).

    0
  • X

    Kann man machen, ich bevorzuge aber ein switch(iID), das macht das erweitern etwas einfacher weil man nicht unbedingt auf die Reihenfolge achten muß.

    0
  • P

    Xebov schrieb:

    Kann man machen, ich bevorzuge aber ein switch(iID), das macht das erweitern etwas einfacher weil man nicht unbedingt auf die Reihenfolge achten muß.

    Natürlich, ich war allerdings davon ausgegangen dass du eine feste Reihenfolge erwartest weil die ReadChunks in deinem Code so fest eingemeißelt waren 😉

    0
  • X

    pumuckl schrieb:

    Xebov schrieb:

    Kann man machen, ich bevorzuge aber ein switch(iID), das macht das erweitern etwas einfacher weil man nicht unbedingt auf die Reihenfolge achten muß.

    Natürlich, ich war allerdings davon ausgegangen dass du eine feste Reihenfolge erwartest weil die ReadChunks in deinem Code so fest eingemeißelt waren 😉

    Ja ich weiß, die Prüfung findet nur im groben statt, im Detail hatte ich das nie Implementiert. Aber sagmal was hast du eig gegen "m_" bei Membern, ich sehe da gerade du schreibst die alle klein.

    0
  • V

    Xebov schrieb:

    Aber sagmal was hast du eig gegen "m_" bei Membern, ich sehe da gerade du schreibst die alle klein.

    es ist nicht mehr notwendig, ja sogar eher störend, wenn man schön kleine funktionen schreibt.

    0
  • P

    Xebov schrieb:

    Aber sagmal was hast du eig gegen "m_" bei Membern.

    Es liefert keinerlei zusätzlich Informationen. Nicht qualifizierte Variablen sind entweder in der Funktion deklariert oder Klassenmember oder global. Globale Variablen sind pfui und daher schlimmstenfalls sehr selten, bestenfalls nicht vorhanden. Lokale Variablen sind in der Funktion, also innerhalb der letzten ~10 Zeilen deklariert und daher muss alles was nicht vor kurzem deklariert wurde ein Klassenmember sein, auch ohne m_. Das m_ stört entweder den Lesefluss, oder man hat sich bereits dran gewöhnt und ignoriert es einfach. Und Information die man ignoriert kann man gleich ganz weglassen. Andersum kann man durchaus mit einem g_ globale Variablen von Klassenmembern absetzen, da sie erstens eh nur selten vorkommen und daher das g_ nicht so leicht ignoriert wird, und zweitens genug Aufmerksamkeit wert sind dass man beim Lesen schon mal drüber stolpern darf.

    0
Anmelden zum Antworten