Qualität des und Fehler im Quelltext

Ich habe mir heute mal die aktuelle Version vom 18.12.2008 heruntergeladen und unter Linux kompiliert. Das funktionierte auch soweit ganz gut, jedoch stürzte das Spiel mit einer Ausnahme ab.

Ich habe dann das Spiel im Debugger unter die Luppe genommen und festgestellt, dass die Funktionen, die für das Einlesen der Textdateien verantwortlich sind, einfach alles durchwinken, ob es nun Dateien, Verzeichnisse oder etwas anderes sind. std::ifstream arbeitet jedoch nur auf Dateien. Unter Linux kommt zudem hinzu, dass alle Verzeichnisse noch . sowie .. als Einträge haben. Ich habe das Problem beseitigt, so dass das Spiel nun auch unter Linux läuft. Im Release Beitrag sind die korrigierten Dateien heruterladbar.

Zudem habe ich noch eine Projektdatei für cmake hinzugefügt, so dass man es bequem kompilieren kann. cmake Dateien sind einfacher zu erstellen, als configure Skripte und haben zudem den Vorteil, dass man damit auch Projektdateien für Visual Studio, KDevelop, XCode und CodeBlocks neben den normalen Makefiles erstellen kann.

Geirroed
Auch wenn es vielleicht etwas unhöflich erscheint, dass ich hier gleich die große Quelltextkeule schwinge, möchte ich folgendes zu deinem bisherigen Werk anmerken:

Ich meine mich zu erinnern, dass du geschrieben hattest, dass du eher aus der C Schiene kommst. Das ist auch anhand des Codes gut zu erkennen.

1. Also prinzipiell ist es bei C++ so, dass jede Klasse in zwei Datein kommt. Jeweils eine h Datei, in der die Deklaration enthalten ist und eine cpp Datei, in der die Implementierung enthalten ist.

2. Es ist wichtig festzulegen, wie die Einrückung aussehen soll. Ob Tabs oder Leerzeichen, ist erstmal egal. Wichtig ist nur, dass wenn man Leerzeichen nimmt, die für jede Einrückstufe identisch sind. Also nicht einmal 6 Leerzeichen, dann wieder 4, dann nur noch 3.

3. Du hast überall struct benutzt. Das ist zwar in Ordnung, bei C++ nimmt man für Klassen aber eigentlich class. Das hat auch den Vorteil, dass man von außen nicht an die Eigenschaften rankommt, wie es bei structs üblich ist. Bei Klassen geht es darum, dass man nur über bestimmte Methoden, die auch Wertüberprüfen übernehmen, auf die Eigenschaften zugreifen darf. Sonst kann man prinzipiell von überall irgendwas an Werten in das jeweilige Objekt schreiben und man wundert sich nachher, warum das Programm nicht das macht, was es soll.

4. C-Stil und Java-Stil... Der C Stil mit _ ist in der Objektorientierung sehr unüblich. In den objektorientierten Sprachen hat sich die Kamelhöckerschreibweise durchgesetzt, die du bei Methodennamen nutzt (das Kleinschreiben des ersten Buchstabens ist Geschmacksache; Java schreibt es klein, C# groß) aber nicht bei Klassennamen. Da solltest du also auch eher, statt combat_stack CombatStack verwenden.

5. Bei Zeigern und Dateideskriptoren ist mir aufgefallen, dass du vergessen hast, den Speicher wieder freizugeben (bei manchen, nicht allen) und das du die Dateideskriptoren nicht schließt. Bei letztem kann man natürlich sagen, dass die eh am Ende der Funktion vernichtet werden. Sauber ist es trotzdem nicht.

Das wäre erstmal alles. Also bitte das ganze als konstruktive Kritik verstehen. Es ist nicht bösartig gemeint.

Zitat

Danke für die Hilfestellung
Wenn sich die Änderungen auf die InitBlablaBuffer() Funktionen und #include stats.h beschränkten, so sind sie jetzt mit eingebunden. Windows kennt zwar ebenso . und .. als Verzeichnisse, hatte aber kein Problem damit den Versuch zu unternehmen ein Verzeichnis mit einem Filestream zu öffnen. Eine genauere Überprfüfung wäre spätestens fällig gewesen, wenn auch die Unterverzeichnisse ausgelesen werden sollen.

Ich muss dazu nochmal etwas richtig stellen. Also das Programm ist unter Linux nicht abgestürzt, sondern mit einer Ausnahme ausgestiegen. Diese sah in der Konsole so aus:

terminate called after throwing an instance of 'std::ios_base::failure'
  what():  basic_filebuf::underflow error reading the file
iconsAborted

Du hast geschrieben, dass du Dev-Cpp nutzt und damit ja logischerweise MingW. Der in MingW enthaltene GCC ist noch eine alte 3er Version, die sich nur unzureichend an den C++ Standard hält. Bei neueren GCC Version (ich nutze momentan 4.2.3) wurde die Konformität zum C++ Standard immer weiter verbessert, so dass dort dann eben Verhaltensweisen auftreten, die beim 3er nicht passieren.

Aber prinzipiell ist es wohl logischer, wenn man erst prüft, ob ein Verzeichniseintrag eine Datei ist, als einfach die Ausnahme zu fangen und dann zu sagen: "Dich wollten wir ja gar nicht."

Und ja. Ich habe nur die Änderungen in der main.cpp gemacht in den von dir oben erwähnten Bereichen.

Zitat

Das ist im Moment leider DevC++ geschuldet. Ich habe zu spät bemerkt, dass das grundsätzlich Tabulatoren in Leerzeichen umwandelt und sich in der Anzahl dabei daran orientiert wo in vorherigen Absätzen bereits welche waren. Ich persönlich empfinde 4 Leerzeichen als gut übersichtlich und passe immer mal wieder die falsch formatierten Stellen an - wie ich halt gerade dazu komme.

Evtl. solltest du dir überlegen, auf eine IDE zu wechseln, die auch weiter entwickelt wird. Ich würde da CodeBlocks empfehlen, zumal cmake eben auch CodeBlocks Projektdateien erzeugen kann. Wenn du Dev Cpp weiter benutzen willst, ist das natürlich auch okay, aber betreute Sachen haben irgendwie einen größeren Charme. :]

Zitat

Ja die Sache mit dem information-hiding ist bislang bei mir noch nicht so richtig drin, warum ich eben erstmal bei der vertrauten (was ja nicht mal stimmt, ist ja trotzdem 'ne Klasse) Struktur geblieben bin. Wird nicht mehr heute, aber in näherer Zeit umgeändert.

Richtig. struct ist fast Synonym zu class zu verwenden, nur das bei class der Zugriffsqualifizierer eben automatisch auf private steht und bei structs auf public. Hat auch damit was zu tun, dass C++ ja rückwärtskompatibel zu C ist. Aber auch in C++ ist es üblich, dass man Strukturen für reine Datentypen nutzt und Klassen eben für ... na ja Klassen eben.

Zitat

Wenn das anderen Programmierern einfachereren/freudigereren Einstieg in das Projekt verschafft, werde ich mich natürlich darum kümmern. Gilt das nur für die Klassennamen, auch die Variablen der Klassen oder auch für die einzelnen Instanzen der Klassen?

Es kommt eben drauf an, welchen Stil man verwenden möchte. Prinzipiell ist aber in der Objektorientierung der Java-Stil üblich. Also eben: Klassennamen groß, Variablen klein, Methoden beginnen mit einem Kleinbuchstaben und insgesesamt wird die Kamelhöckerschreibweise verwendet.
Und dann sollte man sich noch auf sowas wie das Klammersetzen einigen. Ich bin eher ein Freund von: Die öffnende geschweifte Klammer kommt in eine neue Zeile, weil es meiner Meinung nach, die Übersicht fördert. Bei einfachen Konstrukten fällt das nicht so ins Gewicht, aber bei größeren Strukturen, die dann mal länger als eine Bildschirmseite werden, ist es so häufig einfacher zu sehen, in welcher Einrückungsebene man ist. Ist aber, wie gesagt Geschmacksache.

Zitat

5. Bei Zeigern und Dateideskriptoren ist mir aufgefallen, dass du vergessen hast, den Speicher wieder freizugeben (bei manchen, nicht allen) und das du die Dateideskriptoren nicht schließt. Bei letztem kann man natürlich sagen, dass die eh am Ende der Funktion vernichtet werden. Sauber ist es trotzdem nicht.

Das mit den Dateideskriptoren ist recht einfach. Da musst du eben bloß am Ende die close() Methode von std::ifstream aufrufen.

Es gibt unter Linux das Programm valgrind, welches beim Auffinden von Speicherfehlern hilft. Ich füge mal das Ausgabeprotokoll der valgrind Ausgabe vom letzten Release an. Da dort auch Zeilennummern drin stehen, kann man da ganz gut nachschauen.

Also es ist eigentlich immer so, dass wenn du irgendwo einem Zeiger Speicher zuweiset, musst du ihn nachher wieder freigeben. Wenn eine Funktion verlassen wird, werden alle dort instanzierten Variablen freigegeben. Das Problem ist allerdings, dass bei Zeigern nur der Zeiger freigegeben wird, nicht aber das, worauf der Zeiger zeigt. Wenn das passiert, ist der Speicherbereich, auf den der Zeiger zeigte nicht mehr erreichbar und damit verloren.

Bei Zeigern ist es also immer so, dass wenn man sie länger braucht, diese als Klassenvariable (wir gehen mal von C++ aus und da vermeidet man globale Variablen) gespeichert werden und am Ende der Lebenszeit des Objektes, der Speicher im Destruktor der Klasse freigegeben wird.

SDL ist ja selbst in C geschrieben, aber da gibt es ja auch die Methoden wie SDL_FreeSurface (die du schon benutzt), aber eben auch sowas wie SDL_FreeImage.

6. Was mir noch aufgefallen ist: Du benutzt Preprozessordirektiven für Konstanten. Das sollte man bei C++ auch vermeiden, da es dafür Alternativen gibt. Statt also:

#define TIMING_NONE 0

schreibt man

static const short TIMING_NONE = 0;

Das ermöglicht dem Compiler eine bessere Überprüfung und auch unter Umständen eine bessere Optimierung, da const Werte optimiert werden können.

Wobei, wenn ich mir freeroes.h angucke, würden sich für bestimmte Konstanten Aufzählungswerte, sprich enums eher lohnen.

Statt also:

#define ETYPE_CHANGE_ATK 1
#define ETYPE_CHANGE_DEF 2
#define ETYPE_CHANGE_SPEED 3
#define ETYPE_CHANGE_INI 4
#define ETYPE_CHANGE_HP 5
#define ETYPE_BLESSED 6
#define ETYPE_CURSED 7
#define ETYPE_CHANGE_ATB 8
#define ETYPE_TAKE_DAMAGE 10
#define ETYPE_CHANGE_MANA 15

schreibt man:

enum effectType
{
    ETYPE_CHANGE_ATK,
    ...
    ETYPE_TAKE_DAMAGE = 10,
    ETYPE_CHANGE_MANA = 15
};

Dann hat man auch den Vorteil, dass man für deine type Eigenschaft der Klasse Ability, wenn man sie als effectType deklariert, auch nur diese Werte zuweisen kann. Ob das Beispiel jetzt korrekt ist, kann ich nicht sagen, da ich mir den Code nicht angeschaut habe, wo das benutzt wird.