Senet - Altägyptisches Brettspiel

[DelphiMarkus]

Hallo!

Ich möchte euch gerne mein erstes wirkliches Spiel vorstellen. Es ist ein altägyptisches Brettspiel namens Senet, es ist nichts graphisches dabei, alles läuft über die Konsole und Curses. Das Speichern klappt nicht unter Windows (siehe Verbesserungen), aber ansonsten sollte das Spiel laufen. Da das Spiel auch für meinen Palm Pre gedacht war, hab ich darauf geachtet, dass es nicht zu breit wird, soweit ich mich erinnere sollte die curses-Oberfläche mit 80 Zeichen in der Breite schon zufrieden sein.
Die Spielregeln sind von der Bilden Kuh: Blinde Kuh: Senet - Die Spielregeln

Ich hab es im Sommerurlaub programmiert, jeden Abend ein paar Funktionen und Verbesserungen und dabei auch immer Mercurial verwendet. Das Spiel liegt jetzt schon ein paar Tage auf der Platte, weil mir erst nichts mehr eingefallen ist und dann hab ich es einfach vergessen.
Aber ich habe von Anfang an versucht das ganze Spiel möglichst flexibel zu programmieren, mit den Regeln hat das nicht geklappt aber immerhin die Oberfläche ist ohne Änderungen an den Klassen Figur und Spielfeld ersetzbar. Ich bin kein Held wenn es um Aussehen und Oberflächen geht und Curses reicht somit für mich vollkommen aus.

Es gibt noch ein paar Punkte die ich verbessern muss:
Speichern auf Windows ermöglichen ( ich benutze Linux und hab somit erstmal nicht daran gedacht, dass Windows '\' anstatt '/' verwendet )
Spielfeld.ziehen und andere Funktionen entschlacken und Regeln auslagern (pylint meckert über zu viele return's )


Ich freue mich auf Kritik, Anmerkungen und Lob zum Spiel.

Und hier nun der Link zu Code:
Python-Version: 2.6 oder höher
http://www.python-forum.de/pastebin.php?mode=view&s=101

[BlackJack]

@DelphiMarkus: Die Programminfos am Anfang könntest Du `__version__`, `__author__`, und `__date__` nennen. Diese Namen werden, zumindest teilweise, von Programmen wie `pydoc` und `epydoc` besonders berücksichtigt. Datumsangaben würde ich im Format 'YYYY-MM-DD' angeben, dann kann man Datumsangaben auch als Zeichenketten vergleichen, so dass der Vergleich auch zeitlich gesehen Sinn macht.

Das mit dem Pfadtrenner hast Du ja selbst schon angemerkt → `os.path.join()`.

`setfigur()` und `getfigur()` sind unnötig. Man kann stattdessen auch gleich auf `figur` zugreifen.

`Figur` macht zu viel bzw. vereint unterschiedliche Klassen in einer. Immer wenn man per ``if``-Abragen "Typen" unterscheidet, ist das ein deutlicher Hinweis, dass am Entwurf etwas nicht stimmt. Das ganze wird noch "schlimmer" als das es auch noch zwei "Ebenen" gibt, dadurch das eine "Figur" auch Felder repräsentiert.

Warum ist die Statistik ein Dictionary und keine Klasse? Bzw. sind das ja anscheinend Statistiken für die jeweiligen Spieler, gehören also eher zu einem Spieler-Objekt.

Bei `makestat()` sind mehrere Funktionen in einer zusammengefasst. Das `code`-Argument ist unschön. Statt das irgendwo im Code ``makestat(player_nummer, 'z')`` steht, wäre ein ``player.zuege += 1`` an der Stelle wesentlich lesbarer und verständlicher.

`getstat()` ist IMHO überflüssig.

Abkürzungen wie `ret` und `stati` sind IMHO schlechter Stil. Was zum Henker soll `eingabesm` bedeuten?

`ziehen()` ist zu lang und zu unübersichtlich.

Du ziehst anscheinend gerne von Indexen 1 ab. Das riecht ein wenig danach, dass die Datenstruktur falsch aufgebaut ist, oder dass Du GUI und Programmlogik mischst. Wenn in der Anzeige bei 1 angefangen wird zu zählen, darf dass in der Programmlogik nicht dazu führen, dass man da ständig Indexe anpassen muss. Das muss an der Grenze zwischen GUI und Logik passieren, sonst wird es intern früher oder später unübersichtlich.

Du gehst insgesamt zuviel über kryptische Zahlen und Indexe, statt Objekte zu verwenden.

`getrausgeworfen()`, `getjenseits()`, und `getspielerfiguren()` sind wieder Methoden, die auf ein Spieler-Objekt gehören.

In `getspielstand()` steht ein ``except`` ohne konkrete Ausnahme und ausserdem wird dort entweder ein Dictionary oder eine Liste mit zwei Elementen zurückgegeben, die etwas komplett anderes bedeutet. Solche speziellen "Fehlerwerte" sind schlechter Stil. Um diesen Mist loszuwerden wurden Ausnahmen erfunden.

`loadsave()`!? Ja was denn nun? Load oder Save? Wenn man in die Methode reinschaut, keines von beidem. Den kompletten inneren Zustand eines so komplexen Objekts zu setzen, gehört IMHO eher in die __init__(), also dass man eine Funktion oder Klassenmethode schreibt, die ein Dictionary bekommt und daraus ein Spiel-Objekt erstellt.

`Oberflaeche` speichert wohl auch noch ein paar Sachen, die in die Spiellogik gehören. Zum Beispiel die Rundenzahl oder welcher Spieler gerade dran ist. Ebenso ist das Laden und Speichern des Spielstands, und das Würfeln keine Aufgabe der GUI.

Was ist denn das für eine Würfel-Methode!? Warum ist die so kompliziert? Warum hast Du nicht einfach das mit den Stäbchen in Code umgesetzt? Wie kommt man bei einer Beschreibung "Werfe 4 Stäbe die entweder weiss oder rot ergeben können" auf "Ich werfe 10.000 "Stäbe" bei denen weiss, rot, grün, oder gelb herauskommen kann"!? Kannst Du zumindest plausibel erklären warum Dein Code zu den gleichen Wahrscheinlichkeiten kommt, wie das einfache Werfen von vier Stäbchen mit zwei Farben?

[DelphiMarkus]

Erst einmal: Danke für die Rückmeldung BlackJack!
Dann hab ich einiges vor mir, bis das Spiel auch gut aufgebaut ist, aber dazu hab ich es ja auch vorgestellt.

Die Programminfos werde ich wie vorgeschlagen ändern, ebenso wie die Pfadangaben.

`setfigur()` und `getfigur()` sind unnötig. Man kann stattdessen auch gleich auf `figur` zugreifen.

Ich hab gedacht, dass man es besser so machen sollte, ließe sich aber ändern, kann ich auch einsehen.

`Figur` macht zu viel bzw. vereint unterschiedliche Klassen in einer. Immer wenn man per ``if``-Abragen "Typen" unterscheidet, ist das ein deutlicher Hinweis, dass am Entwurf etwas nicht stimmt. Das ganze wird noch "schlimmer" als das es auch noch zwei "Ebenen" gibt, dadurch das eine "Figur" auch Felder repräsentiert.

Hm... Das stimmt schon... Am besten wäre es einmal die Felder zu haben und dann auf den Feldern die Figuren abzustellen. Momentan sind die Figuren ja Feld, Sonderfeld und Figur gleichzeitig...
Diese Typen sind nunmal wichtig für das Spiel... Diese Typen sind eigentlich nur die Hintergründe für das Feld und die muss ich ja auch irgendwo unterbringen. Und für jeden Typ von Feld (Haus der Auferstehung, 5/3/2/1 Felder bis zu Ziel und die Wasserfalle) eine eigene Klasse zu machen rechnet sich im Endeffekt auch nicht wirklich... Dabei haben sie keine Sonderfunktion.

Warum ist die Statistik ein Dictionary und keine Klasse? Bzw. sind das ja anscheinend Statistiken für die jeweiligen Spieler, gehören also eher zu einem Spieler-Objekt.

Es ist ein Dictionary weil ich dachte damit kann man es leicht speichern. Bevor ich das ganze mit JSON gespeichert habe ging das über Prickle...
Aber auf jeden Fall wäre eine Klasse Spieler nicht ganz verkehrt... Ich werde mir mal einige Gedanken dazu machen.

Bei `makestat()` sind mehrere Funktionen in einer zusammengefasst. Das `code`-Argument ist unschön. Statt das irgendwo im Code ``makestat(player_nummer, 'z')`` steht, wäre ein ``player.zuege += 1`` an der Stelle wesentlich lesbarer und verständlicher.

Ich hatte die Wahl zwischen dem 'makestat(spieler, "z")' oder einem 'self.statistik["zuege"][spieler-1] += 1' und da hab ich das erste genommen. Wenn man die Spieler aber in Klassen packt und auch die Statistik, dann würde das wegfallen.

`getstat()` ist IMHO überflüssig.

Das ist das gleiche wie bei 'set-/getfigur()', werde ich ändern.

Abkürzungen wie `ret` und `stati` sind IMHO schlechter Stil. Was zum Henker soll `eingabesm` bedeuten?

'ret', weil ich 'return' ja nicht nehmen kann und 'stati' weil mir 'self.statistik' zu lang war, das gleiche bei eingabesm. Bei letztem kann ich mich aber retten und auf folgende Zeile verweisen: (556)

Code: Alles auswählen

eingabesm = self.getchar() #EingabeSpielMenu

Aber der Name ist tatsächlich nicht sehr schön.

`ziehen()` ist zu lang und zu unübersichtlich.

Ja, ich weiß pylint meckert auch... Ich weiß nur noch nicht wie ich das sinnvoll ändern kann...

Du ziehst anscheinend gerne von Indexen 1 ab. Das riecht ein wenig danach, dass die Datenstruktur falsch aufgebaut ist, oder dass Du GUI und Programmlogik mischst. Wenn in der Anzeige bei 1 angefangen wird zu zählen, darf dass in der Programmlogik nicht dazu führen, dass man da ständig Indexe anpassen muss. Das muss an der Grenze zwischen GUI und Logik passieren, sonst wird es intern früher oder später unübersichtlich.

Ja das war beim Programmieren immer etwas gemein: "Muss da jetzt '- 1' stehen oder nicht...?" Ist bei mir wahrscheinlich ein Überbleibsel von Delphi/Pascal, da hatte ich das auch sehr oft, wenn ich mich recht erinnere...

Du gehst insgesamt zuviel über kryptische Zahlen und Indexe, statt Objekte zu verwenden.

Ich arbeite noch nicht allzu lange mit Klassen, aber sie haben schon immense Vorteile. Ich werde versuchen das in Zukunft zu vermeiden. Allerdings finde ich zu viele Klassen und Objekte auch so unübersichtlich... Das mit den Indexen ist auch ein Überbleibsel von Pascal, dort hatte ich nie so etwas praktisches wie for ... in und so weiter... Ich werde beim Verbessern des Codes daran denken.

`getrausgeworfen()`, `getjenseits()`, und `getspielerfiguren()` sind wieder Methoden, die auf ein Spieler-Objekt gehören.

Was soll ich sagen...: Jap.

In `getspielstand()` steht ein ``except`` ohne konkrete Ausnahme und ausserdem wird dort entweder ein Dictionary oder eine Liste mit zwei Elementen zurückgegeben, die etwas komplett anderes bedeutet. Solche speziellen "Fehlerwerte" sind schlechter Stil. Um diesen Mist loszuwerden wurden Ausnahmen erfunden.

Ich werde sehen, dass ich das loswerde... Und überhaupt: Wenn denn da ein Fehler auftritt, dann bricht das Skript eh ab, denn dort wo ich diese Funktion aufrufe gehe ich davon aus, dass es ein Dictionary zurückliefert. Wenn das nicht der Fall ist, gibt es Ärger.

`loadsave()`!? Ja was denn nun? Load oder Save? Wenn man in die Methode reinschaut, keines von beidem. Den kompletten inneren Zustand eines so komplexen Objekts zu setzen, gehört IMHO eher in die __init__(), also dass man eine Funktion oder Klassenmethode schreibt, die ein Dictionary bekommt und daraus ein Spiel-Objekt erstellt.

Ja, das ist das tolle an Namen... Es wird wohl etwas gemeint sein wie LadeSpielstand... Aber immerhin geladen wird ja ein bisschen, obwohl es ja eher nur Zuweisungen sind, wie z.B. das Spielfeld und die Statistik.

`Oberflaeche` speichert wohl auch noch ein paar Sachen, die in die Spiellogik gehören. Zum Beispiel die Rundenzahl oder welcher Spieler gerade dran ist. Ebenso ist das Laden und Speichern des Spielstands, und das Würfeln keine Aufgabe der GUI.

Ich schaue dann mal, dass ich das trenne... Ich fand es schwer beim Speichern GUI von Logik zu trennen... Die GUI sollte nur zum Anzeigen und Eingeben sein, während die Logik es dann tatsächlich speichert.

Was ist denn das für eine Würfel-Methode!? Warum ist die so kompliziert? Warum hast Du nicht einfach das mit den Stäbchen in Code umgesetzt? Wie kommt man bei einer Beschreibung "Werfe 4 Stäbe die entweder weiss oder rot ergeben können" auf "Ich werfe 10.000 "Stäbe" bei denen weiss, rot, grün, oder gelb herauskommen kann"!? Kannst Du zumindest plausibel erklären warum Dein Code zu den gleichen Wahrscheinlichkeiten kommt, wie das einfache Werfen von vier Stäbchen mit zwei Farben?

Hm... Manchmal ist das genau mein Problem, in Mathe wie in Info... Es klappt alles und das auch gut, aber es ist unnötig. (Schlechtere Noten gab es dadurch noch nicht ) Ich hab mal schlechte Erfahrungen mit Zufallszahlen gemacht und deswegen mache ich immer ganz viele und schaue dann wo von am meisten da ist und das nehme ich dann. Theoretisch müssten ja alle Werte gleich sein, wenn man das statistisch auswerten würde.
Ich war so sehr auf diese 4 Möglichkeiten fixiert, dass ich ganz vergessen habe, dass es nur 2 Werte für jeden Stab gibt. Werde ich dann mal anpassen.

Danke nochmal für die Rückmeldung ich werde sie mir zu Herzen nehmen. Bis zur verbesserten Version wird es etwas dauern, denn ich muss doch einiges umorganisieren.

[BlackJack]

@DelphiMarkus: Die verschiedenen Felder haben keine Sonderfunktion? Warum gibt's die denn dann? (Vielleicht sollte ich die Spielbeschreibung auf Blinde-Kuh doch mal durchlesen. )

`return` wäre ja auch kein guter Name. Was wäre denn ein "zurück"? Das ist ja eher ein Ergebnis und das heisst `result`. Das einem ein Name zu lang ist, sollte kein Grund sein. Quelltext wird deutlich häufiger gelesen als geschrieben und deshalb sollte man mehr Wert auf Verständlichkeit beim Lesen statt Bequemlichkeit beim Schreiben achten. Zumal Autovervollständigung mittlerweile Standard ist. Selbst bei einfachen Texteditoren.

Wenn eine Funktion oder Methode zu lang ist, weil da viele Sachen nacheinander abgeprüft oder umgesetzt werden, dann kann man die zum Beispiel auf mehrere Funktionen aufteilen.

Bei Laden oder Speichern denken die meisten Programmierer an Hintergrundspeicher. Das was Du da in der Methode machst wäre eher so etwas wie `__setstate__()`.

[DelphiMarkus]

Mir fällt gerade ein, dass man, wenn man auf die Wasserfalle kommt, zum Haus der Auferstehung geht und wenn dort schon jemand steht, ins Aus geht. Auf den anderen Sonderfeldern kann man außerdem nicht geschlagen werden.
Aber da die Figuren ja vom Spielfeld verwaltet werden sollen, also auch gesetzt, wüsste ich nicht was ich den Sonderfeldern als zusätzliche Funktionen mitgeben soll. Die Felder sind ja nur dazu zuständig was auf ihnen drauf steht und wie sie aussehen, aber ob und wie gesetzt wird, dass hängt ja da davon ab wie die anderen Figuren auf den Spielfeld stehen.
Da hab ich mich vorhin zwar etwas vertan , als ich gesagt habe, dass diese Felder keine Sonderfunktion haben, aber die Sonderstellungen die sie haben, können sie eigentlich nicht alleine verwalten.
Wenn ich dabei falsch liege, lasse ich mich gerne eines besseren belehren.

Was mir noch einfällt wäre, dass man für ein Feld eine Variable setzt, ob die Figur auf dem Feld geschlagen werden kann oder nicht. Damit könnte man wahrscheinlich die ziehen()-Methode etwas kleiner machen.

[BlackJack]

@DelphiMarkus: Ich habe bis jetzt vier Klassen für die Logik: `Player`, `Field`, `Board`, und `Game`.

Ein `Field` weiss welcher Player (oder keiner) auf ihm steht und ob das Feld "sicher" ist. Ausserdem gibt es ein Property mit dem Namen `is_free` was angibt, ob das Feld von einem Stein besetzt ist oder nicht. Und eine Methode, die prüft ob das Feld ein mögliches Zugziel für einen gegebenen Spieler ist.

[DelphiMarkus]

Hallo!

Da es ja sehr viel zu verbessern gibt, was den Code angeht, habe ich mich entschlossen das Ganze nochmal komplett neu zu schreiben und dieses Mal auf die Punkte, die BlackJack angemerkt hat, zu achten. Damit ich nicht sofort wieder "alles" falsch mache , stelle ich jetzt mal den Code den ich bisher neu geschrieben habe zur Schau. Damit ich schon am Anfang eine Rückmeldung dazu bekomme, ob das so in Ordnung geht, bevor ich wieder alles fertig habe.
Deswegen fänd ich es echt nett, dass mir nochmal jemand eine Rückmeldung gibt.

Bisher ist die Figur-Klasse soweit fertig und das Spielfeld kann sich selbst auch schon "anzeigen":
http://www.python-forum.de/pastebin.php?mode=view&s=109

Ich hoffe das ist jetzt etwas besserer Code als vorher.

@BlackJack:
Ich hab mal versucht, das Feld so zu schreiben, wie du es vorgeschlagen hast. Ich hoffe es entspricht ungefär deinen Vorstellungen. :K

[BlackJack]

@DelphiMarkus: Ich habe im `Field` bei mir nur den Spieler (oder `None`) und ob man auf dem Feld sicher ist, gespeichert. Die `render()`-Geschichte gehört IMHO schon zur GUI und nicht mehr zur Logik.

`Feld.pruefe_zug()` ist viel zu langatmig. Diese vielen ``return``\s mit `True` und `False` sind doch total unnötig -- die Bedingungen werden doch schon zu `True` oder `False` ausgewertet. Das kann man direkt als Ergebnis zurück geben.

Mein `Field` sieht so aus:

Code: Alles auswählen

class Field(object):
    def __init__(self, is_safe):
        self.player = None
        self.is_safe = is_safe
    
    @property
    def is_free(self):
        return self.player is None
    
    def is_possible_target(self, player):
        return self.is_free or (player is not self.player and not self.is_safe)

So etwas wie `setze_spieler()` braucht man dann nicht, denn `is_free` ist kein Attribut sondern wird bei der Abfrage ermittelt.

Ich denke in `Spielfeld` machst Du es Dir auch nur unnötig kompliziert, dass Du die `felder` nicht einfach in der "richtigen" Reihenfolge speicherst. Dass das als drei Reihen mit der mittleren Reihe von rechts nach links *dargestellt* wird, ist für die reine Spiellogik doch völlig egal. Das ist auch eine GUI-Angelegenheit.