Hallo erst einmal.
Ich habe schon vor langer Zeit begonnen das Kartenspiel Herzeln (siehe http://de.wikipedia.org/wiki/Herzeln) in Python zu schreiben und es jetzt vollendet. Ich glaube es sollte richtig funktionieren, aber es hat momentan nur ein Kommandozeileninterface. Ich wäre nichtsdestotrotz sehr dankbar falls sich irgendjemand die Mühe machen könnte, mir Tipps zu geben wie ich das Programm verbessern kann.
Der Code ist hier : https://gist.github.com/prog-r-amer/66b ... stfile1-py
Herzeln Kartenspiel
Hallo,
ich antworte in Form von Stichpunkten; erstens fällt es mir leichter so zu schreiben und zweitens kann man es auch leichter lesen und evtl. wie eine to-do-Liste abhaken. Versteh das bitte nicht als unhöflich.
ich antworte in Form von Stichpunkten; erstens fällt es mir leichter so zu schreiben und zweitens kann man es auch leichter lesen und evtl. wie eine to-do-Liste abhaken. Versteh das bitte nicht als unhöflich.
- Trenne Logik und Output (eine Klasse, in der weder input noch print in den Methoden vorkommt und eine, die nur für Interaktion zuständig ist). Das macht es einfacher zu testen und neue Interfaces wie GUI oder Web hinzuzufügen. Außerdem solltest du dir überlegen, wie du vorgehen wirst, wenn du weitere Sprachen unterstützen möchtest (nur den Code dazu vorbereiten).
- Zeile 12: wirf eine Exception (ValueError oder eine eigene Exception) statt sys.exit zu rufen. Wieder das Argument mit dem Testen und außerdem mit der Semantik
- Zeile 28: wenn dicts immer gleich definiert werden, sieht das stark nach einer Konstante aus. Lager das aus auf Klassen- oder Modulebene
- tour8 und tour machen ziemlich viel. Schau mal, wie man da einige Prozeduren auslagern kann
Hallo,
weitere Stichpunkte
weitere Stichpunkte
- CARDNAMES ist eine Konstante und sollte nicht verändert werden. Kommt später ein vierter Spieler hinzu, kann der nicht mehr mitspielen, weil Du die 7er und 8er schon zerstört hast
- in __init__ sollten aller Attribute einmal gesetzt werden und nicht noch weitere in verschiedenen Methoden dazukommen. Das macht es einfacher zu sehen, welche Attribute eine Klasse überhaupt haben kann.
- Du solltest überlegen eine Player-Klasse zu erzeugen und nicht die Eigenschaften eines Spielers auf verschiedene dicts verteilen.
- __init__ sollte nur initialisieren und zwar vollständig (s.o.), aber nur das. »tour« aufzurufen ist dann Aufgabe des Hauptprogramms. Rekursive Aufrufe sind kein Ersatz für eine Schleife.
-
BlackJack
@prog(r)amer: (Einiges wurde schon gesagt, ich habe den Text hier nebenbei über einen längeren Zeitraum geschrieben.)
Ausgiebig getestet ist das nicht. Es gibt Fehler.
Zum Beispiel hat die Klasse ein Attribut `pcount`, in `tour8()` wird allerdings versucht auf `p_count` zuzugreifen, was es nicht gibt. Ebenfalls dort wird auf `players` und `player_sums` als lokale Namen zugegriffen, obwohl das Attribute sind.
Ein weiterer Fehler ist es bei drei Spielern etwas auch `CARDNAMES` zu löschen. Das ist eine Konstante, die darf man nicht verändern. Wenn man einmal ein `Game`-Objekt mit drei Spielern erstellt hat, dann kann man kein weiteres mehr erstellen was nicht mit falschen Daten arbeitet. Egal mit wie vielen Spielern.
Warum gibt es diese „magische” API bei `Game.__init__()`? Man hätte da auch ohne den ``*`` auskommen können und einfach eine Liste mit Spielernamen übergeben können.
`COLORS` würde auf Englisch eher `SUITS` heissen.
Das `pcount`-Attribut ist redundant. Das ist ein Kandidat für ein `property()` damit man nicht darauf achten muss, dass die beiden Informationen `pcount` und `players` immer synchron bleiben. Man sollte es auch nicht abkürzen sondern `player_count` nennen, damit man beim lesen nicht erst raten muss, was es denn heissen mag.
Die Klasse `Game` enthält viele Informationen und viele/lange Methoden — sehr wahrscheinlich zu viel. Das zusammen damit dass man gar kein sinvolles Exemplar von der Klasse erstellen kann, macht das eigentlich zu nichts anderem als Funktionen die auf globalem Zustand operieren. Also ein schlecht geschriebenes Modul in eine Klasse verschoben.
Dafür spricht auch das `sys.exit()` in der `__init__()`-Methode. Eine Klasse die man flexibel benutzen können soll darf niemals einfach das gesamte Programm abbrechen dürfen. An der Stelle kann man eine Ausnahme auslösen und der Aufrufer kann dann entscheiden ob er das Programm abbricht, oder den Benutzer vielleicht auffordert eine andere Anzahl von Spielern zu wählen.
Ebenfalls schlechter Klassenentwurf ist das einführen von Attributen ausserhalb der `__init__()`. Nach ablauf dieser Methode sollte ein Objekt in einem vollständig initialisiertem Zustand sein. Mit allen Attributen die den Zustand von einem Exemplar dieses Datentyps ausmachen.
Viele Zeilen sind länger als 80 Zeichen.
So oft wie `CARDNAMES.index()` aufgerufen wird, scheint eine Liste nicht die richtige Datenstruktur zu sein. Insgesamt macht das Programm, auch wegen der `Game`-„god class” nicht den Eindruck als sei es von den kleineren Bausteinen her aufgebaut, die dann getestet und zu einem grösseren Ganzen zusammengesetzt wurden. Denn So etwas wie ein Datentyp für eine Karte die man mit einer anderen Karte vergleichen kann, wäre dabei IMHO eine recht naheliegende Sache gewesen.
Bei den Abbildungen die `player` als Schlüssel verwenden stellt sich die Frage ob der Spieler nicht auch ein Datentyp sein sollte, der Attribute wie Name, Punktanzahl, Karten und so weiter hat, statt diese Informationen auf viele Datenstrukturen zu verteilen. Das `players`-Attribut sollte so wie es jetzt ist, deshalb auch besser `player_names` heissen, denn hinter `players` würde man Objekte erwarten die einen kompletten Spieler mit allen seinen Eigenschaften repräsentieren und nicht nur einen Namen.
Mehrere Methoden die ``if``/``elif``-Kaskaden auf dem selben Wert durchführen, wie beispielsweise die Nummer der aktuellen Tour sind ein „code smell”. Das ist kein guter objektorientierter Entwurf. Die verschiedenen Regeln bei den Touren sollte man in eigene Objekte heraus ziehen. Eventuell reichen hier übrigens auch Funktionen aus wenn eine Tour keinen eigenen Zustand hat der nicht schon im Spiel-Objekt abgebildet werden muss. Ähnlich wie bei einem `Player`-Datentyp kann man in einem `Tour`-Objekt jeweils die Sachen zusammen fassen die zu einer Tour gehören, wie die Beschreibung, die Berechnung von Punkten, eventuell eine Kartenvergleichsfunktion die bestimmte Regeln der jeweiligen Tour kodiert, und so weiter. Der Code in den einzelnen Funktionen und Methoden wird dadurch einfacher und übersichtlicher.
`deal_out()` müsste keine Methode sein, das hätte man auch prima als Funktion definieren können. Alternativ als Methode einer `CardDeck`-Klasse. Und statt des Sonderfalls abhängig von der Anzahl der Spieler und den hart kodierten Zahlen, hätte man auch eine allgemeine Schleife über die Spieler samt Index (mit `enumerate()`) schreiben können. Der Name `parted_cards` ist falsch. Ich denke darunter kann sich kein englischssprechender Mensch vorstellen was gemeint ist. Bei Abbildungen hat sich bewährt eine Bezeichnung für Schlüssel und Wert verbunden durch ein `_to_` oder eine `2` zu verwenden, also in diesem Fall zum Beispiel `player_name2hand` oder `player_name2cards`. Wobei wie das wie schon gesagt eine Information ist, die eigentlich in ein Spielerobjekt gehört.
`clockwise_sort()` ist ineffizient. Die bessere Datenstruktur hier wäre eine `collections.deque` statt einer Liste, denn der Queue-Datentyp hat eine effiziente Methode um die Elemente zu „rotieren”. Hier würde sich wohl auch anbieten die Spieler in einem `Players`-Objekt zu kapseln was die Reihenfolge regelt. Da kann man dann die konkrete Implementierung (einfache Liste, `collections.deque`, verkette Liste von Spielerobjekten) ändern, ohne dass das den Rest des Quelltextes beeinflusst.
`tour()` ist für meinen Geschmack zu lang und zu kompliziert. Sehr wahrscheinlich erledigt sich das von selbst wenn man all die Sachen die eigentlich als Operationen auf andere Datentypen gehören, dorthin verschiebt. Gehört `decision()` dort überhaupt rein? Es benutzt nichts von `Game`. Ich habe jetzt nicht geschaut ob dort etwas aus dem Namensraum der umgebenden Methode verwendet wird. Sollte es eigentlich nicht, und falls doch könnte man es als Argument übergeben.
Der Typ des Rückgabewerts von `decision()` ist unschön. Entweder eine Karte oder `False`? Warum `False`? Der Benutzer *muss* ja eine Entscheidung treffen also sollte die Funktion auch erst einen Wert zurückgeben wenn es eine gültige Auswahl gibt. Rückgaberwerte einer Funktion oder Methode sollten immer vom gleichen „duck type” sein, denn sonst wird der aufrufende Code komplizierter, weil der prüfen muss, was er da eigentlich bekommen hat.
Der Code zum „hübschen” Formatieren/Ausgeben von Karten kommt zwei mal im Quelltext vor. Das könnte man in eine Funktion heraus ziehen. Oder in jeweils eine `__str__()`-Methode auf einem Karten- und einem Hand-Datentyp.
Auch bei solchen Zugriffen wie ``trick[0]['card'][0]`` sollte man dringend über mehr Datentypen nachdenken. Ab drei verschachtelten Index/Schlüsselzugriffen wird das nämlich sehr unübersichtlich und man weiss nicht was die „magischen” Indizes bedeuten. An der Stelle wäre ``trick[0].card.suite`` leichter zu lesen und zu schreiben. Noch besser vielleicht ``trick.first_card.suite``.
Rekursive Aufrufe als Ersetz für einfache Schleifen zu missbrauchen ist unschön. In Sprachen in denen so etwas nicht garantiert weg optimiert wird, müllt das unnötig den Speicher voll und kann irgendwann zum Absturz führen wenn der Aufrufstapel voll ist.
Ausgiebig getestet ist das nicht. Es gibt Fehler.
Zum Beispiel hat die Klasse ein Attribut `pcount`, in `tour8()` wird allerdings versucht auf `p_count` zuzugreifen, was es nicht gibt. Ebenfalls dort wird auf `players` und `player_sums` als lokale Namen zugegriffen, obwohl das Attribute sind.
Ein weiterer Fehler ist es bei drei Spielern etwas auch `CARDNAMES` zu löschen. Das ist eine Konstante, die darf man nicht verändern. Wenn man einmal ein `Game`-Objekt mit drei Spielern erstellt hat, dann kann man kein weiteres mehr erstellen was nicht mit falschen Daten arbeitet. Egal mit wie vielen Spielern.
Warum gibt es diese „magische” API bei `Game.__init__()`? Man hätte da auch ohne den ``*`` auskommen können und einfach eine Liste mit Spielernamen übergeben können.
`COLORS` würde auf Englisch eher `SUITS` heissen.
Das `pcount`-Attribut ist redundant. Das ist ein Kandidat für ein `property()` damit man nicht darauf achten muss, dass die beiden Informationen `pcount` und `players` immer synchron bleiben. Man sollte es auch nicht abkürzen sondern `player_count` nennen, damit man beim lesen nicht erst raten muss, was es denn heissen mag.
Die Klasse `Game` enthält viele Informationen und viele/lange Methoden — sehr wahrscheinlich zu viel. Das zusammen damit dass man gar kein sinvolles Exemplar von der Klasse erstellen kann, macht das eigentlich zu nichts anderem als Funktionen die auf globalem Zustand operieren. Also ein schlecht geschriebenes Modul in eine Klasse verschoben.
Dafür spricht auch das `sys.exit()` in der `__init__()`-Methode. Eine Klasse die man flexibel benutzen können soll darf niemals einfach das gesamte Programm abbrechen dürfen. An der Stelle kann man eine Ausnahme auslösen und der Aufrufer kann dann entscheiden ob er das Programm abbricht, oder den Benutzer vielleicht auffordert eine andere Anzahl von Spielern zu wählen.
Ebenfalls schlechter Klassenentwurf ist das einführen von Attributen ausserhalb der `__init__()`. Nach ablauf dieser Methode sollte ein Objekt in einem vollständig initialisiertem Zustand sein. Mit allen Attributen die den Zustand von einem Exemplar dieses Datentyps ausmachen.
Viele Zeilen sind länger als 80 Zeichen.
So oft wie `CARDNAMES.index()` aufgerufen wird, scheint eine Liste nicht die richtige Datenstruktur zu sein. Insgesamt macht das Programm, auch wegen der `Game`-„god class” nicht den Eindruck als sei es von den kleineren Bausteinen her aufgebaut, die dann getestet und zu einem grösseren Ganzen zusammengesetzt wurden. Denn So etwas wie ein Datentyp für eine Karte die man mit einer anderen Karte vergleichen kann, wäre dabei IMHO eine recht naheliegende Sache gewesen.
Bei den Abbildungen die `player` als Schlüssel verwenden stellt sich die Frage ob der Spieler nicht auch ein Datentyp sein sollte, der Attribute wie Name, Punktanzahl, Karten und so weiter hat, statt diese Informationen auf viele Datenstrukturen zu verteilen. Das `players`-Attribut sollte so wie es jetzt ist, deshalb auch besser `player_names` heissen, denn hinter `players` würde man Objekte erwarten die einen kompletten Spieler mit allen seinen Eigenschaften repräsentieren und nicht nur einen Namen.
Mehrere Methoden die ``if``/``elif``-Kaskaden auf dem selben Wert durchführen, wie beispielsweise die Nummer der aktuellen Tour sind ein „code smell”. Das ist kein guter objektorientierter Entwurf. Die verschiedenen Regeln bei den Touren sollte man in eigene Objekte heraus ziehen. Eventuell reichen hier übrigens auch Funktionen aus wenn eine Tour keinen eigenen Zustand hat der nicht schon im Spiel-Objekt abgebildet werden muss. Ähnlich wie bei einem `Player`-Datentyp kann man in einem `Tour`-Objekt jeweils die Sachen zusammen fassen die zu einer Tour gehören, wie die Beschreibung, die Berechnung von Punkten, eventuell eine Kartenvergleichsfunktion die bestimmte Regeln der jeweiligen Tour kodiert, und so weiter. Der Code in den einzelnen Funktionen und Methoden wird dadurch einfacher und übersichtlicher.
`deal_out()` müsste keine Methode sein, das hätte man auch prima als Funktion definieren können. Alternativ als Methode einer `CardDeck`-Klasse. Und statt des Sonderfalls abhängig von der Anzahl der Spieler und den hart kodierten Zahlen, hätte man auch eine allgemeine Schleife über die Spieler samt Index (mit `enumerate()`) schreiben können. Der Name `parted_cards` ist falsch. Ich denke darunter kann sich kein englischssprechender Mensch vorstellen was gemeint ist. Bei Abbildungen hat sich bewährt eine Bezeichnung für Schlüssel und Wert verbunden durch ein `_to_` oder eine `2` zu verwenden, also in diesem Fall zum Beispiel `player_name2hand` oder `player_name2cards`. Wobei wie das wie schon gesagt eine Information ist, die eigentlich in ein Spielerobjekt gehört.
`clockwise_sort()` ist ineffizient. Die bessere Datenstruktur hier wäre eine `collections.deque` statt einer Liste, denn der Queue-Datentyp hat eine effiziente Methode um die Elemente zu „rotieren”. Hier würde sich wohl auch anbieten die Spieler in einem `Players`-Objekt zu kapseln was die Reihenfolge regelt. Da kann man dann die konkrete Implementierung (einfache Liste, `collections.deque`, verkette Liste von Spielerobjekten) ändern, ohne dass das den Rest des Quelltextes beeinflusst.
`tour()` ist für meinen Geschmack zu lang und zu kompliziert. Sehr wahrscheinlich erledigt sich das von selbst wenn man all die Sachen die eigentlich als Operationen auf andere Datentypen gehören, dorthin verschiebt. Gehört `decision()` dort überhaupt rein? Es benutzt nichts von `Game`. Ich habe jetzt nicht geschaut ob dort etwas aus dem Namensraum der umgebenden Methode verwendet wird. Sollte es eigentlich nicht, und falls doch könnte man es als Argument übergeben.
Der Typ des Rückgabewerts von `decision()` ist unschön. Entweder eine Karte oder `False`? Warum `False`? Der Benutzer *muss* ja eine Entscheidung treffen also sollte die Funktion auch erst einen Wert zurückgeben wenn es eine gültige Auswahl gibt. Rückgaberwerte einer Funktion oder Methode sollten immer vom gleichen „duck type” sein, denn sonst wird der aufrufende Code komplizierter, weil der prüfen muss, was er da eigentlich bekommen hat.
Der Code zum „hübschen” Formatieren/Ausgeben von Karten kommt zwei mal im Quelltext vor. Das könnte man in eine Funktion heraus ziehen. Oder in jeweils eine `__str__()`-Methode auf einem Karten- und einem Hand-Datentyp.
Auch bei solchen Zugriffen wie ``trick[0]['card'][0]`` sollte man dringend über mehr Datentypen nachdenken. Ab drei verschachtelten Index/Schlüsselzugriffen wird das nämlich sehr unübersichtlich und man weiss nicht was die „magischen” Indizes bedeuten. An der Stelle wäre ``trick[0].card.suite`` leichter zu lesen und zu schreiben. Noch besser vielleicht ``trick.first_card.suite``.
Rekursive Aufrufe als Ersetz für einfache Schleifen zu missbrauchen ist unschön. In Sprachen in denen so etwas nicht garantiert weg optimiert wird, müllt das unnötig den Speicher voll und kann irgendwann zum Absturz führen wenn der Aufrufstapel voll ist.
-
prog(r)amer
- User
- Beiträge: 10
- Registriert: Freitag 4. Januar 2013, 12:57
Danke erst einmal für die vielen Verbesserungsvorschläge. Ich werde dann bald einmal eine neue Version hier posten.
Ganz allgemeine Dinge:
Der Shebang sollte env befragen und keine Annahmen über den Installationsort vom Interpreter treffen.
Mal abgesehen davon, warum soll der Code nur mit 3.3 funktionieren?
Auch die Encoding Zeile sparst du dir.
Der Shebang sollte env befragen und keine Annahmen über den Installationsort vom Interpreter treffen.
Mal abgesehen davon, warum soll der Code nur mit 3.3 funktionieren?
Auch die Encoding Zeile sparst du dir.