Highscore-Klasse

Hundefutter

Hallo zusammen.
Schreibe gerade an einer Klasse zur Verwaltung eines Highscore-Systems in einem Spiel. Die Klasse soll möglichst unabhängig sein, so dass sie in mehreren Spielen verwendet werden kann.
Ich möchte hier schon einmal einen ersten Entwurf des Klassendesigns zeigen, um Rückmeldungen diesbezüglich zu erhalten. So kann ich schnell Fehler oder Probleme erkennen und Verbesserungen einfließen lassen.

Also die Klasse soll eine Liste verwalten, welche die Highscore repräsentiert.
Mit den nötigen Zugriffsoperationen wird diese Liste dann verändert und das Dateihandling vorgenommen.
Hier mein erster Entwurf (bestimmt noch nicht vollständig, aber ein erster Anfang):

#ifndef CHighscore_h
#define CHighscore_h

#include <list>

struct HighscoreEntry
{
	char name[30];
	int punkte;	
};

class CHighscore
{
	public:
		CHighscore(); //liest Liste aus Datei ein
		virtual ~CHighscore(); //schreibt Liste wieder in Datei

		//Die Liste nach "außen" hin geben (-> Ausgabe o.ä.)
		void getHighscoreList(std::list<HighscoreEntry> &pList);

		//neuen Eintrag in die Liste einfügen
		void AddnewEntry(HighscoreEntry pNew);

	protected:
		std::list<HighscoreEntry> mHighscoreList;		
};

#endif //CHighscore_h

Ich denke, die Kommentare sind ausreichend und erklären gut, wie ich mit die Aufgabenverteilung in den einzelnen Funktionen vorgestellt habe.

Ich hoffe auf Anmerkungen und Kritik (oder auch Zustimmung ;)).

MfG
Hundefutter

Fusel.Factor

Woher weiß der Construktor denn welche Datei er öffnen muss?
Ich würde GetHighscoreList eher so machen:

std::list<HighscoreEntry>& getHighscoreList( void );
// oder, je nachdem wie du es haben willst
const std::list<HighscoreEntry>& getHighscoreList( void ) const;

Damit kann man besser arbeiten.

Bei AddNewEntry kannst du den Parameter auch als Referenz übergeben.

void AddnewEntry( const HighscoreEntry& pNew);

Wofür steht das p vor deinen Variablen?
Verwende lieber std::string anstatt char[]

Badestrand

Fusel.Factor schrieb:

std::list<HighscoreEntry>& getHighscoreList( void );
// oder, je nachdem wie du es haben willst
const std::list<HighscoreEntry>& getHighscoreList( void ) const;

Das halte ich für ziemlich ziemlich schlecht. Im Prinzip hat die Idee was, aber einer der Vorteile von Kapselung ist ja, dass man die dahinter-liegende Implementation auswechseln kann, ohne dass der "Benutzer" der Klasse was davon mitbekommt oder sogar seinen Code ändern muss. Wenn du nun eine Referenz zurückgibst, kannst du später nicht mehr die std::list durch z.B. einen std::vector oder ein std::set ersetzen, da du dann keine list-Referenz mehr hast.
Wenn du hingegen ein Objekt statt einer Referenz zurückgibst, kannst du intern ruhig die Liste durch einen vector ersetzen, die Schnittstelle funktioniert trotzdem noch.

Nexus

Zu den bereits gesagten Dingen möchte ich noch einige Vorschläge hinzufügen:

Für eine Highscore würde ich nicht std::list , sondern std::set bzw. std::multiset verwenden. Du musst dann den Operator < (als Default; hängt vom Sortierkriteriums-Funktor ab) für deine Eintragsklasse überladen, dafür werden die Einträge automatisch sortiert.
Das Struct würde ich innerhalb der Klasse, und zwar im privaten Bereich definieren. Es werden ja keine Einträge ausserhalb der Highscore benötigt.
Hast du deine Klasse zur Ableitung vorgesehen? Ansonsten sind virtual und protected unpassend.

camper

@Badestrand: So ganz überzeugend ist das in meinen Augen nicht. Immerhin wirkt sich die Rückgabe eines neuen Containers negativ auf die Laufzeitkomplexität aus, auch nimmt man dann unnötig die Möglichkeit von Exceptions in Kauf. Wenn die Implementation wirklich so instabil ist, sollte vielleicht eher die Verwendung eines Proxies oder Views in Erwägung gezogen werden.

Dravere

Badestrand schrieb:

Das halte ich für ziemlich ziemlich schlecht. Im Prinzip hat die Idee was, aber einer der Vorteile von Kapselung ist ja, dass man die dahinter-liegende Implementation auswechseln kann, ohne dass der "Benutzer" der Klasse was davon mitbekommt oder sogar seinen Code ändern muss. Wenn du nun eine Referenz zurückgibst, kannst du später nicht mehr die std::list durch z.B. einen std::vector oder ein std::set ersetzen, da du dann keine list-Referenz mehr hast.
Wenn du hingegen ein Objekt statt einer Referenz zurückgibst, kannst du intern ruhig die Liste durch einen vector ersetzen, die Schnittstelle funktioniert trotzdem noch.

Das ist zwar wahr, aber da gibt es eine Zwischenlösung:

class Foo
{
  // Typedefs //
public:
  typedef std::list<Entry> Container_t;

  // Attributes //
private:
  Container_t m_container;

  // Methods //
public:
  Container_t& get_container() { return m_container; };
  Container_t const& get_container() const { return m_container; };
};

So kann man std::list zum Beispiel durch einen std::vector oder ein std::set ersetzen. Eine Zwischenlösung ist es deswegen, weil man die Sache nicht zum Beispiel durch eine std::map ersetzen kann.

Grüssli

Badestrand

@camper: Ich finde, bei Klassen wo die Laufzeit so ziemlich egal ist (wie hier, wenn nicht sehr viele Einträge drin sind), kann man ruhig Objekte zurückgeben. Zudem dürfte eine Get-Highscore-Funktion ja nicht in einem aufwendigen Algorithmus zum Tragen kommen, so dass sie (in meinen Augen) eigentlich nie ein Flaschenhals sein kann.
Die Gefahr von Exceptions sehe ich hier auch nicht, immerhin haben wir die Kontrolle des Container-Templ.-Parameters und es kommen nur ziemlich simple Typen zum Einsatz.
Wenn man aber generell Objekte zurückgibt (wie ich das immer mache), stimmen die Argumente natürlich voll und ganz!

@Dravere: Warum dann nicht gleich einen öffentlichen Container_t-Member, nimmt sich dann ja nichts mehr?

Ich weiß auch nicht, aber ich hab wirklich eine grundsätzliche Abneigung gegen Referenz-Rückgabe Klar ist die Objekt-Rückgabe wesentlich langsamer, aber das entreißt mir irgendwie zu stark die Kontrolle; ist vielleicht ein Tick von mir oder so

Dravere

Badestrand schrieb:

@Dravere: Warum dann nicht gleich einen öffentlichen Container_t-Member, nimmt sich dann ja nichts mehr?

Naja, ich muss zugeben, bei meinen Klassen biete ich normalerweise nur diese Methode an:

Container_t const& get_container() const { return m_container; };

Und das einfach aus dem Grund, da es so praktisch ist, alle Einträge zu durchlaufen. Egal in welche Richtung und ich nicht alle Methoden vom Container wrappen muss.

Wenn ich die völlig offene Methode anbiete:

Container_t& get_container() { return m_container; }

Dann kommt natürlich sowieso die andere dazu. Viel wichtiger ist aber, dass ich dann keine spezielle Kontrolle über die Liste haben will. Es ist also alles erlaubt mit der Liste zu machen. Sonst müsste ich alle Funktionen wrappen und dazu habe ich keine Lust.

In dem Fall der Klasse hier, würde ich übrigens vorschlagen, dass man sie vollständig entfernt. Die Klasse CHighscore ist wahrscheinlich nur ein Wrapper für ein std::multiset . Daher wäre es klüger die Klasse HighscoreEntry auszubauen und wie es bereits Nexus vorgeschlagen hat, mit einem operator < auszustatten.

Grüssli

Hundefutter

So, erstmal Danke für die große Anzahl an Rückmeldungen.

Die Absicht der Klasse war, sie völlig unabhängig von anderem Code in den Spielen zu schreiben, eben so, dass intern in der Klasse alles verwaltet wird.
Daher würde ich auch lieber eine Klasse nehmen, welche die komplette Verwaltung der Highscore übernimmt.
Von außen soll man dann nur Objekte eingeben und die Ausgabe der Highscore zurückkriegen können. Mehr sollte ich ja eigentlich nicht brauchen.

Um die Sortierung der Liste muss ich mir eingentlich keine Gedanken machen, da neue Objekte sortiert eingefügt werden und so die Liste immer sortiert ist.

Über die Abhängigkeit von std::list denke ich nochmal nach, obwohl ich finde, dass eine Liste völlig ausreichend ist, da diese einfach zu durchlaufen ist und auch immer sortiert vorliegt. Werde aber gucken, ob ich mir offen halte, den Container noch zu ändern...

Ist die Variante, dass der Konstruktor die Liste aus der Datei läd und der Destruktor die Liste wieder in die Datei schreibt, so gut?
So gehe ich sicher, dass diese Operationen auf jeden Fall durchgeführt werden, was sie ja auch müssen.

MfG
Hundefutter

Fellhuhn

Eine Möglichkeit die Scores manuell zu speichern ist sicherlich auch sinnvoll. Gerade dann wenn das Spiel auch gerne noch mal anfangs crasht und der Destruktor nicht aufgerufen wird.

Ansonsten würde ich eine Templateklasse draus machen.

Und zwar sollte der Name vom Typ std::string sein und der eigentliche Highscorewert der Templateparameter. So funktioniert es dann mit int, float... Vielleicht noch ein Datum dazu? Als Timestamp.

So kannst du auch dann die get-Methode gestalten:

bool getEntry(int num, std::string &name, T &value, unsigned int &timestamp){
  if (list.size() <= num) return false;

  name = ...
  value = ... 
  timestamp = ...

  return true;
}

Hundefutter

Fellhuhn schrieb:

Eine Möglichkeit die Scores manuell zu speichern ist sicherlich auch sinnvoll. Gerade dann wenn das Spiel auch gerne noch mal anfangs crasht und der Destruktor nicht aufgerufen wird.

Die Spiele selber sind soweit fertig, sie sollten eigentlich nicht mehr abstürzen, der Highscore ist nun nur eine kleine Erweiterung der Spiele..
Denke also, ich lasse es erstmal im Destruktor. Wäre natürlich auch keine große Sache, das in eine Methode zu packen, die ich dann munuell aufrufe.

Fellhuhn schrieb:

Ansonsten würde ich eine Templateklasse draus machen.

Und zwar sollte der Name vom Typ std::string sein und der eigentliche Highscorewert der Templateparameter. So funktioniert es dann mit int, float... Vielleicht noch ein Datum dazu? Als Timestamp.

So kannst du auch dann die get-Methode gestalten:
bool getEntry(int num, std::string &name, T &value, unsigned int &timestamp){
  if (list.size() <= num) return false;

  name = ...
  value = ... 
  timestamp = ...

  return true;
}

Das mit dem Template werde ich mal versuchen, so bin ich natürlich noch unabhängiger. Das mit dem Datum ist auch eine gute Idee, das werde ich wohl mit aufnehmen..

Was meint ihr? Brauche ich denn jetzt noch mehr als das Hinzufügen neuer Einträge und die Ausgabe der Highscore?
Diese beiden Funktionen werde ich auf jeden Fall schon mal möglicht komfortabel und unabhängig implementieren.

MfG
Hundefutter

Nexus

Hundefutter schrieb:

Um die Sortierung der Liste muss ich mir eingentlich keine Gedanken machen, da neue Objekte sortiert eingefügt werden und so die Liste immer sortiert ist.
[...]
Über die Abhängigkeit von std::list denke ich nochmal nach, obwohl ich finde, dass eine Liste völlig ausreichend ist, da diese einfach zu durchlaufen ist und auch immer sortiert vorliegt.

Da verwechselst du was. std::list ist eine doppelt verkettete Liste, also ein sequenzieller und kein assoziativer Container. Da wird auch nichts automatisch sortiert, sonst wären wohl Methoden wie push_front() und push_back() relativ sinnlos. Wie gesagt, nimm für einen automatisch sortierten Container ein std::set oder std::multiset , wenn mehrere identische Schlüssel vorkommen dürfen. Da kann man auch die maximale Anzahl Werte überwachen:

while (set_container.size() > 5)
    set_container.erase(set_container.begin());

Hundefutter schrieb:

Was meint ihr? Brauche ich denn jetzt noch mehr als das Hinzufügen neuer Einträge und die Ausgabe der Highscore?
Diese beiden Funktionen werde ich auf jeden Fall schon mal möglicht komfortabel und unabhängig implementieren.

Vielleicht noch eine Funktion zum Zurücksetzen der Highscore.

S.T.A.L.K.E.R.

Warum fängt die Methode getHighscore mit einem kleinen g an und die Methode AddnewEntry mit einem großen a? Und warum ist das n bei new klein? "addnew" ist kein englisches Wort, das sind zwei Wörter, also sollte es AddNew sein.

Ich vote übrigens für addNewEntry statt GetHighscore

Außerdem würde ich etwas mehr Arbeit investieren und nicht einfach die Datenstruktur in der die Highscores gespeichert werden zurückgeben, sondern die einzelnen Einträge bzw. einen Iterator mit dem man diese durchgehen kann.
Außerdem würde ich einen op<< und op>> hinzufügen um die Highscore bequem speichern und laden zu können
Ebenso einen Konstruktor der einen std::istream entgegen nimmt und aus diesem die Highscore lädt.

Hundefutter

Nexus schrieb:

Da verwechselst du was. std::list ist eine doppelt verkettete Liste, also ein sequenzieller und kein assoziativer Container. Da wird auch nichts automatisch sortiert, sonst wären wohl Methoden wie push_front() und push_back() relativ sinnlos.

Mir ist klar, dass die Liste nicht automatisch sortiert wird, nur da ich den neuen Eintrag immer sofort sortiert einfüge, liegt sie immer sortiert vor und sie nie unsortiert, also muss auch nichts sortiert werden.

Nexus schrieb:

Vielleicht noch eine Funktion zum Zurücksetzen der Highscore.

Jo, das könnte noch mit aufgenommen werden.

S.T.A.L.K.E.R. schrieb:

Außerdem würde ich etwas mehr Arbeit investieren und nicht einfach die Datenstruktur in der die Highscores gespeichert werden zurückgeben, sondern die einzelnen Einträge bzw. einen Iterator mit dem man diese durchgehen kann.
Außerdem würde ich einen op<< und op>> hinzufügen um die Highscore bequem speichern und laden zu können
Ebenso einen Konstruktor der einen std::istream entgegen nimmt und aus diesem die Highscore lädt.

Jo, werde ich auch versuchen mit zu implementieren.

Mache mich gleich mal dran, weiter an der Klasse zu arbeiten.
Schreibe dann später wieder die aktuelle Version.

MfG
Hundefutter

Dravere

Darf ich ein zwei Vorschläge anbringen?

class HighscoreEntry
{
private:
  std::string m_name;
  long m_timestamp;
  long m_points;

public:
  HighscoreEntry(std::string const& name, long points, long timestamp)
    : m_name(name)
    , m_points(points)
    , m_timestamp(timestamp)
  { }

  ~HighscoreEntry() { };

public:
  std::string const& get_name() const { return m_name; };
  void set_name(std::string const& name) { m_name = name; };

  long get_timestamp() const { return m_timestamp; };
  void set_timestamp(long timestamp) { m_timestamp = timestamp; };

  long get_points() const { return m_points; };
  void set_points(long points) { m_points = points; };
};

inline bool operator <(HighscoreEntry const& left, HighscoreEntry const& right)
{
  return (left.get_points() == right.get_points() ?
           left.get_name().compare(right.get_name()) < 0 :
           left.get_points() < right.get_points());
}

typedef std::multiset<HighscoreEntry> Highscore_t;

inline void write_highscore(std::string const& path, Highscore_t const& highscore)
{
  /* Schreibfunktion */
}

inline void read_highscores(std::string const& path, Highscore_t& highscore)
{
  highscore.clear();

  /* ... lese die Highscore ... */
}

Wäre meiner Meinung nach am einfachsten, nicht?
std::multiset übernimmt auch die Sortierung deiner Einträge voll automatisch und sortiert sie schneller ein, als deine std::list das kann. Denn du musst die Liste sequentiell durchlaufen, um den Eintrag zu platzieren.

Grüssli

Hundefutter

Hui, das ist ja schon fast eine komplette Lösung..
Danke für deine Mühe.
Finde die Lösungsart sehr gut und werde es nun selbst versuchen ähnlich zu implementieren. Ich merke selbst, dass mir noch sehr viel Übung fehlt, gerade im Umgang mit der STL..
Also versuche ich das Ganze erstmal selbst, kann mir ja dann deine Lösung hier angucken, falls ich nicht weiterkomme.

Vielen Dank an alle für die Hilfen.

MfG
Hundefutter

Hundefutter

Ich habe jetzt für mich so eine Klasse geschrieben, deren Objekte Einträge der Highscore sind.
Nun habe ich versucht, mit multiset zu arbeiten, nur irgendwie läuft bei mir der insert-Befehl nicht.

std::multiset<HighscoreEntry> Highscore; //multiset vom Typ HighscoreEntry
std::multiset<HighscoreEntry>::iterator it; //entsprechender Iterator

std::string name;
int points;

std::cout << "Name: ";
std::cin >>  name;
std::cout << "Punkte: ";
std::cin >> points;

HighscoreEntry Entry(name, points); //Instanz der Klasse

Highscore.insert(Entry); //Zeile, die den Fehler verursacht

Beim Kompilieren meckert er (siehe unten).
Kann es sein, dass der Vergleichstyp hier noch nicht definiert ist? Der Container muss ja nach einem bestimmten Kriterium sortiert werden und ich denke mir, er weiß bei dem Typ nicht, wie er es machen soll...

Hier die Fehlerausgabe:

make -k all
Building file: ../main.cpp
Invoking: GCC C++ Compiler
g++ -O0 -g3 -Wall -c -fmessage-length=0 -MMD -MP -MF"main.d" -MT"main.d" -o"main.o" "../main.cpp"
/usr/include/c++/4.1.3/bits/stl_function.h: In member function »bool std::less<_Tp>::operator()(const _Tp&, const _Tp&) const [with _Tp = HighscoreEntry]«:
/usr/include/c++/4.1.3/bits/stl_tree.h:857: instantiated from »typename std::_Rb_tree<_Key, _Val, _KeyOfValue, _Compare, _Alloc>::iterator std::_Rb_tree<_Key, _Val, _KeyOfValue, _Compare, _Alloc>::insert_equal(const _Val&) [with _Key = HighscoreEntry, _Val = HighscoreEntry, _KeyOfValue = std::_Identity<HighscoreEntry>, _Compare = std::less<HighscoreEntry>, _Alloc = std::allocator<HighscoreEntry>]«
/usr/include/c++/4.1.3/bits/stl_multiset.h:310: instantiated from »typename std::_Rb_tree<_Key, _Key, std::_Identity<_Key>, _Compare, typename _Alloc::rebind<_Key>::other>::const_iterator std::multiset<_Key, _Compare, _Alloc>::insert(const _Key&) [with _Key = HighscoreEntry, _Compare = std::less<HighscoreEntry>, _Alloc = std::allocator<HighscoreEntry>]«
../main.cpp:20: instantiated from here
/usr/include/c++/4.1.3/bits/stl_function.h:227: Fehler: no match für »operator<« in »__x < __y«
make: *** [main.o] Fehler 1
make: Das Target »all« wurde wegen Fehlern nicht aktualisiert.
Build complete for project Highscore-Klasse

Was meint ihr dazu? Wo liegt hier noch der Fehler?

MfG
Hundefutter

Dravere

Steht ja in der Fehlermeldung, obwohl die wirklich sehr mühsam sind, die GCC-Fehlermeldungen:

Fehler: no match für »operator<« in »__x < __y«

Also der operator < fehlt für die Klasse HighscoreEntry . Du musst also einen entsprechenden operator < definieren. Ein Beispiel dafür siehst du in meinem Beispiel oben

Grüssli

Hundefutter

Ich habe nun mal versucht, deine Definition des Operators einzufügen:

inline bool operator <(HighscoreEntry const &left, HighscoreEntry const &right);

Nun sagt er mir aber immer, dass die Funktion nur ein Element nehmen darf:

../HighscoreEntry.h:18: Fehler: »bool HighscoreEntry::operator<(const HighscoreEntry&, const HighscoreEntry&)« muss genau ein Argument nehmen

Warum sagt der das?

Dravere

Ich habe den operator < ausserhalb der Klasse definiert, wie man es bei diesen Operatoren oft macht. Du dagegen, hast den operator < in der Klasse definiert. Wenn du ihn in der Klasse definierst, dann hast du als erster Wert this , also das eigentliche Objekt und als zweiter Wert, den übergebenen wert. Wenn du noch einen zweiten Parameter definierst, hättest du eigentlich 3 Werte zum vergleichen, was mit dem operator < nicht geht. deshalb die Fehlermeldung.

Also entweder raus aus der Klasse damit oder wenn du ihn in der Klasse behälst, dann einen Parameter weniger und mit this vergleichen.

Grüssli

Hundefutter

Ach klar, dummer Fehler...

Naja, dann ist ja alles klar.

Danke für die Hinweise.

MfG
Hundefutter