Schiffe versenken auf pythonisch

[ntrunk]

Hi,

hier ein kleines Spiele-Projekt zum Anschauen, Kritisieren und Erweitern:
http://www.python-forum.de/pastebin.php?mode=view&s=53
Wer hat Lust, weitere Computer-Player zu schreiben oder ein UI zu programmieren? (Das Snippet muss in zwei Dateien aufgeteilt werden, steht im Code als Kommentar)

Gruß
Norbert

[ms4py]

Zuerst mal: nette Idee, gefällt mir

Nun aber die Kritik: Deine `threading` Lösung mit dem Schießen ist sehr unschön (um es vornehm auszudrücken). Was hast du dir denn dabei gedacht?! Das geht doch auch mit `while` ohne eine Rekursion (Pseudo-Code):

Code: Alles auswählen

...
while not self.is_finished:
    while self._current_player.can_shoot:
        self._current_player.shoot()
    self.next_player()
...
def next_player(self):
    self._current_player.can_shoot = True
    self._current_player = other_player

Noch ein paar Ideen: Oberfläche mit pygame, Netzwerkmodus (z.B. mit Pyro)

Edit: Was auch noch aufgefallen ist. Du machst das ganze IMO unnötig viel zu kompliziert mit deinen Datenstrukturen. Vor allem steckt da auch Redundanz, z.B. in den Positionen der Schiffe. Wie ich das ganze angehen würde:
- ocean: 2-D Array mit 3 Zuständen: Wasser, Schiff, getr. Schiff (=> ersetzt Ship und Ocean zum Teil)
- shots: 2-D Array mit 2 Zuständen: geschossen oder nicht (True/False) (=> ersetzt Ocean._shots)
Jedes Array wird je einmal einem Player zugeordnet. Spieler hat verloren wenn sein `ocean` kein Feld mehr mit `Schiff` enthält. Spieler sieht von seinem Gegenspieler genau das, worauf er schon geschossen hat, wenn mal also seine `shots` mit dem `ocean` des anderen Spieler verknüpft.

Damit reduzierst du die Komplexität auf mindestens ein Drittel des bisherigen Codes, wenn nicht noch weniger. Außerdem werden deine Laufzeiten deutlich besser, die Auswertung eines Schusses auf eine Position ist damit z.B. in O(1) erledigt.

[BlackJack]

@ntrunk: Das blockierende Threading scheint mir etwas umständlich zu sein. Was ist der Sinn davon neue Threads zu starten, die letztendlich sequentiell abgearbeitet werden. Oder habe ich da etwas übersehen?

Statt `SHOT_NOT_ALLOWED` hätte man dort mit einer Ausnahme arbeiten können.

Für die Spieler hätte ich ein boolean verwendet. ``opposite = player^1`` ist IMHO ziemlich kryptisch für Leute die kein Assembler oder C gewohnt sind. Und Pascal-Programmierer fragen sind warum dort `player` in die erste Potenz erhoben wird. ``opposite = not player`` finde ich verständlicher. Da `bool` eine Unterklasse von `int` ist, und `True` und `False` den Zahlenwerten 1 und 0 entsprechen, kann man die auch als Index in die `_oceans` verwenden.

`Ocean._shots` könnte an ein `set()` binden, statt einer Liste. Das würde ein wenig "Linearität" aus der Laufzeit nehmen, und den Quelltext geringfügig vereinfachen.

`Ocean.is_finished()` geht in einer Zeile:

Code: Alles auswählen

    def is_finished(self):
        """ergibt True, wenn alle Schiffe versenkt wurden, sonst False"""
        return all(ship.is_sunken() for ship in self._ships)

Ich versuche möglichst in der `__init__()` alle Attribute für das Objekt zu definieren, dann sieht man schneller welche es auf dem Objekt später geben wird.

`shipsleft` könnte man mit einer "list comprehension" initialisieren:

Code: Alles auswählen

        shipsleft = [Ship(size) for _ in xrange(n)
                                for size, n in sorted(ships.iteritems())]

Iiiih, die unnötige Rekursion in `_set_ship()` ist hässlich. So etwas sollte man sich in Python nicht angewöhnen, da stösst man früher oder später an das Rekursionslimit. Zumal das hier sehr verschleiernd wirkt IMHO. Es gibt eine Methode `_set_ships()` die kein Schiff setzt, sondern dafür `_set_ship()` aufruft, was anders als der Name vermuten lässt *alle* Schiffe setzt, und das auch noch durch rekursive Aufrufe.

Read-Only per `property()` erzwingen finde ich unschön. `Ship.size` hätte man auch einfach als normales Attibut realisieren können. Wer käme denn auf die Idee das zu ändern? Und wenn, dann sollte ein Hinweis in der Dokumentation ausreichen.

`Ship._hits` könnte man wieder in einem `set()` verwalten.

[ms4py]

Hab oben noch einmal was geschrieben, falls das untergegangen sein sollte

[jbs]

Was macht `^` denn?

[cofi]

Code: Alles auswählen

>>> 1 ^ 1
0
>>> 0 ^ 1
1

XOR, oder mit anderen Worten: Etwas das man der Lesbarkeit halber besser vermeidet.

Code: Alles auswählen

player = (player + 1) % len(players)

Halte ich fuer klarer (ausserdem kann man dann auch noch vom klassischen 2 Spieler System abweichen )

[ntrunk]

Hi zusammen,

zunächst mal Danke an alle für eure konstruktive Kritik (zum Thema Kritik: die Guten halten es aus, und um die anderen ist es eh nicht schade... )

@ms4py
Thema Threading:
Das Threading war tatsächlich eines der größeren Probleme beim Entwurf. Darauf gekommen bin ich, weil für die Implementierung eines menschlichen Spielers asynchrones Auswerten eines Schusses erforderlich wird. Deshalb informiert zunächst die World den Player, dass er jetzt einen Schuss abgeben kann mit "on_generate_shot". Diese Methode würde beim HumanPlayer nichts tun. Irendwann später, sobald sich der menschl. Spieler entschieden hat, kommt dann der Aufruf der shoot-Methode vom UI. Denke ich da noch zu kompliziert?

Thema UI:
Ja, da kann ich mir auch eine Menge vorstellen Vllt. mach ich zunächst mal eins für die Konsole.

Thema Redundanz und Komplexität:
Naja, dass ich oft zu kompliziert denke, ist mir auch schon des öfteren aufgefallen... Die jetzige Implementierung hat sich eigentlich automatisch dadurch ergeben, dass ich das Ganze objektorientiert angegangen bin und mir überlegt habe, was ein Objekt ist. Dadurch ist das Objekt "Ship" entstanden, das über seine eigene Position Bescheid weiß und die ganzen Operationen auf dem Schiff handelt.

@BlackJack:
Thema Threading:
siehe oben bei ms4py

Thema Exception:
Du hast recht, da wäre eine Exception besser, da der Rückgabe eigentlich einen Fehler meldet.

Thema Rekursion:
Die Namensgebung der Funktionen zum Schiffe setzen ist wirklich nicht optimal. Die Rekursion selbst verwende ich, um bei einem fehlgeschlagenen Positionierungsversuch möglichst einfach den vorigen Zustand des Ozeans wiederherstellen zu können. Die Rekursiontiefe entspricht dabei maximal der Anzahl der Schiffe, das sollte kein Problem darstellen, zumal mir das iterativ zu schreiben wesentlich komplizierter zu sein scheint.

Thema ^ vs. not:
Hey, der Ausdruck "opposite = not player" gefällt mir, und wie die Antwort von jbs zeigt ist es tatsächlich lesbarer.

Thema List-Comprehensions, Generatoren:
Damit kann ich scheinbar noch einiges vereinfachen in meinem Code, danke.

Gruß
Norbert

[Hyperion]

Ohne mir den Code genau angeguckt zu haben: Schiffe versenken ist doch vom Ablauf her rein sequentiell? Damit sehe ich keinen Nutzen von Threading... erst schießt der eine, dann der andere. Oder übersehe ich hier was?

[BlackJack]

Und wenn es ein GUI-Frontend gibt, hat das dafür zu sorgen, dass ein menschlicher Spieler keine Schüsse absetzen kann, während er nicht an der Reihe ist. Wenn man einen Server aus dem Spiel bastelt, um das auch über's Netz spielen zu können, muss die Spiellogik das natürlich überprüfen, damit keiner schummeln kann.

[ntrunk]

@Hyperion und BlackJack:
Kann es sein, dass ich gerade dabei bin, eine Schraube mit dem Hammer einzudrehen? Mir fehlt das nötige Pattern, wie ich den linearen Ablauf der Game-Engine mit dem asynchronen Ablauf einer GUI in Einklang bringe.
Neuer Anlauf:
Die run-Methode ändere ich wie folgt:

Code: Alles auswählen

#----- 8< -----
    def run(self):
        current_player = 0
        while not self.is_finished():
            self.shot_done = False
            self.players[current_player].on_generate_shoot():
            while not self.shot_done:
                pass
#----- 8< -----

Die bisherige next_shot - Methode entfällt, und der Statusanzeiger wird dann jeweils am Ende der shoot-Methode gesetzt. Für mein Beispiel mit den beiden Computer-Playern ist das Ok, in einer Gui würde aber der Ablauf blockiert. Wie kann ich das lösen? Kann ich die run-Methode in einem eigenen Thread laufen lassen? Da stehe ich noch auf dem Schlauch...

Gruß
Norbert

[BlackJack]

@ntrunk: Wenn Du da eine GUI drauf setzt, muss diese das Spiel vorantreiben. Eine "langlaufende" `run`-Methode mit einer Schleife, die solange läuft bis das Spiel beendet ist, geht da nicht. Du musst die API so bauen, dass man "schrittweise" von aussen den Spielverlauf vorantreiben kann.

[ntrunk]

@ntrunk: Wenn Du da eine GUI drauf setzt, muss diese das Spiel vorantreiben. Eine "langlaufende" `run`-Methode mit einer Schleife, die solange läuft bis das Spiel beendet ist, geht da nicht. Du musst die API so bauen, dass man "schrittweise" von aussen den Spielverlauf vorantreiben kann.

Ich glaube, jetzt wird langsam ein Schraubenzieher aus meinem Hammer:
Die Initiative geht nicht von der Engine aus, sondern von der GUI. Die GUI holt sich also von der Engine die Information, wer an der Reihe ist und ruft dann die generate_shot - Methode des jeweiligen Computerplayers auf bzw. gibt den Schuss des menschlichen Spielers an die Engine weiter, sobald der entsprechend geklickt hat (was es anscheinend bei mir jetzt auch gemacht hat, danke )

Gruß
Norbert