Wieviele LOC für Funktion?

Xebov

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.

Shade Of Mine

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?

pumuckl

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

Xebov

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.

pumuckl

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

Xebov

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

pumuckl

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

Xebov

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.

volkard

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.

pumuckl

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.