Sokoban-Spielchen mit Pygame

Stellt hier eure Projekte vor.
Internetseiten, Skripte, und alles andere bzgl. Python.
Antworten
Astorek
User
Beiträge: 72
Registriert: Samstag 24. Januar 2009, 15:06
Kontaktdaten:

Hi @ all,

Tjoar, ich habe mich mal in den letzten Tagen drangesetzt und einen "Sokoban"-Klon geschrieben. Für alle die das Spiel nicht kennen: Es geht darum, Kisten auf ihre vorherbestimmten Plätze zu schieben - allerdings lässt sich nur eine Kiste gleichzeitig verschieben, und ein Ziehen von Kisten ist nicht möglich.

Zu meiner Verteidigung ( ;) ) muss ich gleich sagen: Ursprünglich komme ich aus der QBasic-Ecke und habe viel (zuviel) Zeit mit dieser Sprache verbracht. Ich hatte beim Programmieren mit Python mehrmals das Gefühl, dass ich es mir komplizierter gemacht habe, als es wahrscheinlich nötig gewesen wäre^^. Kurz: Der Quellcode könnte mit Sicherheit besser sein, als er es aktuell ist^^... (Vorallem mit Klassen habe ich, als ich das Projekt begonnen hatte, quasi nur theoretische Erfahrungen gehabt... Ich bin mir nichtmal sicher, ob ich die Klassen in dem Maße sinnvoll nutze, wie sie eigentlich gedacht waren^^).

Hier mal ein Screenshot:


Bild


Es gibt einige Einstellmöglichkeiten, z.B. bezüglich Auflösung und Sound. Dazu muss man die settings.py bearbeiten, dort ist alles kommentiert oder selbsterklärend...

Die Levels stammen von einem gewissen David W. Skinner, der seine Levels gegen Creditwürdigung kostenlos vertreibt.
("These sets may be freely distributed provided they remain properly credited.", http://users.bentonrea.com/~sasquatch/sokoban/ )

Hier der Downloadlink: http://astorek.bplaced.net/downloads/sokoban.zip
Wär super, wenn einige ihr Feedback abgeben würden :)

EDIT: Ganz vergessen, der zu startende Quellcode wäre dann die "_startme.py"...
Dauerbaustelle
User
Beiträge: 996
Registriert: Mittwoch 9. Januar 2008, 13:48

Ohne das Programm jetzt ausgeführt zu haben:
  • Für den JSON-Kram solltest du etwas verwenden wie

    Code: Alles auswählen

    try:
        import json
    except ImportError:
        import simplejson as json
    Bitte keine Import-Error abfangen, nur um dann eine eigene Fehlermeldung zu geben -- am nützlichsten für den Nutzer ist der pure Fehler :-)
  • Du willst sicherlich in Zukunft PEP8 befolgen.
  • die `as`-Import-Abkürzungen finde ich ziemlich doof, weil `loadlevel` besser lesbar ist als `ll`.
  • `waitOnKey`: sollte bestimmt `waitForKey` heißen, außerdem ist die `run`-Variable unnötig -- du kannst direkt in der `while`-Schleife `break`en oder `return`en.
  • Diese `main`-Funktion ist mir irgendwie ein Dorn im Auge. Man (bzw. Ich) versteht kein Wort davon, sie ist viel zu lang und so weiter. Die Haupt-Programm-Logik sollte nicht in `main` stehen, sondern auf viele, kleine Funktionen/Methoden/Klassen verteilt.
  • in `dumbmenu` bietet sich ggf. ein Dictionary für den Code ab Zeile 193 an. (Dieses ewig lange `elif`-Zeug)
  • loadlevel.py: Ich verstehe den Sinn der Klasse nicht ganz -- wenn du eh nur eine Instanz davon erzeugst? Die leere `__init__`-Methode ist unnötig. Für Python < 3 sollten alle Klassen von `object` erben. Was machen Zeilen 214 und 215? Die Konstante `None` wird laut Styleguide nicht mit `==`, sondern mit `is` verglichen. In Zeile 213 sollte einfach eine Exception geraised werden. (-1)-Return-Werte macht man in Python nicht wirklich, dafür gibt es Exceptions. Private Variablen macht man laut Styleguide mit EINEM Unterstrich, nicht mit zwei.
So, das waren jetzt hauptsächlich formale Bemerkungen und Styleguide-Abgeklappere. Aber auch inhaltlich: Logik besser verteilen, so versteht den Code kein Mensch.
BlackJack

@Astorek: So ganz grob: Namen sind oft zu kurz und Funktionen oft zu lang.

Im Gegensatz zu QBasic reicht es in Python nicht eine Funktion oder Methode einfach nur "hinzuschreiben", man muss sie auch tatsächlich *aufrufen*. Denn ein aufrufbares Objekt zu referenzieren und es aufzurufen sind in Python zwei voneinander trennbare Schritte, da Funktionen und Methoden auch Objekte sind, und man sie zum Beispiel an andere Namen binden oder als Argumente übergeben kann. Also ``spam = eggs`` ist etwas anderes als ``spam = eggs()``. Im ersten Fall wird das Objekt, das an den Namen `eggs` gebunden ist, auch an den Namen `spam` gebunden, und im zweiten Fall wird das Objekt `eggs` aufgerufen und das *Ergebnis* an den Namen `spam` gebunden. Sollte es keine Zuweisung geben, sondern nur die rechte Seite des Ausdrucks, dann ist der erste Fall einfach nur eine Referenzierung von `eggs` ohne einen Effekt. So etwas hast Du zum Beispiel in `dumbmenu()` mit der Zeile `pygame.font.init`. Das macht nichts aus, weil man diese Funktion normalerweise sowieso nicht selbst aufrufen muss, wenn man davor schon mal `pygame.init()` aufgerufen hat.

`dumbmenu()` ist so eine Funktion die zu lang ist. Das mit den Nummern und Buchstaben ist ein wenig undurchsichtig. Wenn man das erste mal `char` sieht, wird der Wert erhöht, man hat aber noch gar nicht die erste Bindung gesehen.

Den Namen `i` in einer Schleife für etwas anderes als eine Zählvariable zu verwenden ist keine gute Idee. Das verwirrt gerade erfahrene Programmierer ungemein, denn `i`, `j`, und `k` sind in sehr vielen Sprachen traditionelle Namen für ganzahlige Zählvariablen in Schleifen. Da `cursorpos` "manuell" parallel zu den Menüpunkten hochgezählt wird, kann man das mittels `enumerate()` auch in die Schleife integrieren. Oder man könnte die Positionen als Generator schreiben. Man könnte den Teil zum Beispiel so formulieren (`count()` und `izip()` aus `itertools`):

Code: Alles auswählen

    labels = string.digits[1:] + string.ascii_uppercase
    positions = (size // distance * i for i in count())
    for position, label, text in izip(positions, labels, menu):
        rendered_text = myfont.render('%s. %s' % (label, text),
                                      True,
                                      fgcolor)
        textrect = rendered_text.get_rect()
        textrect = textrect.move(x_pos, position  + y_pos)
        screen.blit(rendered_text, textrect)
        pygame.display.update(textrect)
Vergleiche auf `True` und `False` sind überflüssig wenn das, was man damit vergleicht sowieso schon ein Wahrheitswert ist.

Auf `exitMenu` kann man wohl auch ganz verzichten, wenn man stattdessen an den entsprechenden Stellen einfach ein ``return`` verwendet.

Der "hässliche lange Teil" kann mit einem Dictionary verkürzt werden, dass `pygame.K_*`-Konstanten auf die entspechenden Werte abbildet. Mehrere Anweisungen mit Semikolon getrennt sind in Python nicht so gerne gesehen. Das ist in der Regel "Platz" sparen auf Kosten der Lesbarkeit. Gleiches gilt für Code direkt nach ``:`` statt den eingerückt in die nächste Zeile zu schreiben.

Die "wrap around"-Logik für den Menücursor kann man mit Modulo mit weniger Tests erreichen. Einfach bei den entsprechenden Tastendrücken `cursorpos` anpassen und am Ende mit ``cursorpos %= len(menu)`` dafür sorgen, dass der Wert im erlaubten Bereich landet.

Bei der `loadLevel`-Klasse solltest Du Dich nochmal mit Objektorientierung beschäftigen. Die ist grauenvoll. Objekte sollten nach dem erstellen benutzbar sein. Alle Attribute des Objekts sollten in der `__init__()` gebunden werden -- zumindest an einen Platzhalter, damit man als Leser sieht aus was solche Objekte eigentlich bestehen.

Der Name `pos` ist in dem Modul ziemlich verwirrend, weil ich das im Kopf irgendwie immer in `position` expandiere, aber anscheinend zumindest nicht immer Positionen damit gemeint sind!?

Die vielen doppelten Unterstriche wurden ja schon erwähnt.

Warum werden `__returnvalue` und `__yf`, in `getPos()` an das Objekt gebunden? Was bedeutet der Index 2 in dieser Methode? Solche magischen Zahlen machen Programme undurchsichtig.

Die ganzen `set*`-Methoden, die einfach nur Attribute auf die übergebenen Werte setzen sind unpythonisch. Die kann man auch direkt setzen. Triviale "getter" wie `getArray()` sind auch nicht nötig.

In `fillArray()` werden wieder *alle* Namen an das Objekt gebunden. Methoden zu verwenden bedeutet doch nicht, dass man keine lokalen Namen mehr verwenden darf. Die Methode ist auch zu lang und zu kompliziert. Ich hab's jetzt nicht genau durchblickt, aber ich habe auch den Eindruck Du hast es irgendwie mit "anlegen von Arrays" und die dann mittels Indexzugriffen mit Werten zu füllen. An der Stelle baut man die Listen in Python in der Regel dynamisch auf.

Der Name von `_startme.py` ist ungünstig weil widersprüchlich, denn ein führender Unterstrich bedeutet per Konvention in Python "Implementierungsdetail, bitte nicht direkt verwenden".

Pfade plattformunabhängig zusammensetzen geht mit `os.path.join()`. Das kommt auch damit klar, wenn eine Plattform mehr als einen Pfadtrenner unterstützt und die in den Teilen gemischt vorkommen.

Die Werte bei `lev.setVals()` sehen mir eher nach einem Einsatz für ein Dictionary aus. Und die Zahlen, die den Indizes entsprechen sehen mir irgendwie redundant aus!? Und es wären auch wieder so "magische" Zahlen, bei denen man gar nicht weiss, was sie bedeuten, wenn man ihnen ohne weitere Erklärung begegnet. Die ganzen `set*()`-Aufrufe hätte man sich auch sparen können, wenn die Klasse eine `__init__()`-Methode hätte, der man die Werte hätte übergeben können.

Dass die `main()` *viel* zu umfangreich ist, wurde ja schon gesagt.

Das Menü selbst und einige Menüpunkte bringen Textzeilen auf den Bildschirm und da wiederholt sich ein guter Teil Quelltext, den man in eine Funktion auslagern könnte.

Das die "Nicht-Programme" eine Meldung ausgeben, wenn man sie als solche starten will, ist zwar IMHO überflüssig, aber nicht so schlimm. Das sich das Programm aber aktiv dagegen wehrt als Modul importiert zu werden ist Unsinn. Das kann Dokumentationsgeneratoren und Testprogramme behindern.
Benutzeravatar
mkesper
User
Beiträge: 919
Registriert: Montag 20. November 2006, 15:48
Wohnort: formerly known as mkallas
Kontaktdaten:

Achso: Lass dich von den vielen Kritikpunkten nicht unterkriegen, die sind nur gutgemeint! 8)
Astorek
User
Beiträge: 72
Registriert: Samstag 24. Januar 2009, 15:06
Kontaktdaten:

Vielen Dank für die ganzen Vorschläge, da habe ich noch ein wenig Arbeit vor mir :) .

Ich gestehe, meine größten Erfahrungen in Python und Pygame habe ich mir aus dem Tutorial "A Byte of Python" und dem Zusammenkratzen der entsprechenden Pygame-Funktionen in der "Index Reference" angeeignet. Nach kurzer Recherche finde ich jetzt v.a. die Dictionarys erst richtig interessant, weil genanntes Tutorial die Mächtigkeit jener Dicts nicht wirklich vermitteln konnte...
Dicts werden jetzt für mich wirklich interessant :shock:
BlackJack hat geschrieben:Im Gegensatz zu QBasic reicht es in Python nicht eine Funktion oder Methode einfach nur "hinzuschreiben", man muss sie auch tatsächlich *aufrufen*. Denn ein aufrufbares Objekt zu referenzieren und es aufzurufen sind in Python zwei voneinander trennbare Schritte, da Funktionen und Methoden auch Objekte sind, und man sie zum Beispiel an andere Namen binden oder als Argumente übergeben kann. Also ``spam = eggs`` ist etwas anderes als ``spam = eggs()``. Im ersten Fall wird das Objekt, das an den Namen `eggs` gebunden ist, auch an den Namen `spam` gebunden, und im zweiten Fall wird das Objekt `eggs` aufgerufen und das *Ergebnis* an den Namen `spam` gebunden. Sollte es keine Zuweisung geben, sondern nur die rechte Seite des Ausdrucks, dann ist der erste Fall einfach nur eine Referenzierung von `eggs` ohne einen Effekt.
Vielen Dank für die Erläuterung, nun ist mir Einiges klarer geworden :) .
Dauerbaustelle hat geschrieben:
  • loadlevel.py: Ich verstehe den Sinn der Klasse nicht ganz -- wenn du eh nur eine Instanz davon erzeugst?
Ich gestehe, "loadLevel" stammt noch aus meinen QBasic-Zeiten, die ich damals rein prozedural verwendet habe... Ich habe mir wohl noch zuwenig Gedanken darüber gemacht, als ich diese Funktion in OOP auf Python "portiert" hatte... As say, zuwenig praktische Erfahrung mit Klassen. Ich muss ich an den "Gedankengang" bei Klassen, erst recht bei Spielen, noch gewöhnen^^.
Was machen Zeilen 214 und 215?
Ups, da hat sich in Zeile 215 eine vergessene Variable eingeschlichen^^. 214 sorgt dafür, dass die While-Schleifen in 217 und 246 durchlaufen werden - die ändert sich erst in "False", wenn der Spieler das Spiel beenden will, dadurch landet der Spieler wieder im Hauptmenü der allerersten While-Schleife in 107. Wenn ich genauer drüber nachdenke, habe ich die Variable eigentlich falsch benannt, "RunGame" wäre wohl ein besserer Name als "MainMenu", das beschreibt ja genau das Gegenteil^^.

Nochmals ein Dankeschön an euch beiden für die ganzen Fehler, die ich definitiv noch ausbessern will. :)

@mkesper: Danke, dessen bin ich mir voll und ganz bewusst. 8)
Dauerbaustelle
User
Beiträge: 996
Registriert: Mittwoch 9. Januar 2008, 13:48

Astorek hat geschrieben:
Was machen Zeilen 214 und 215?
Ups, da hat sich in Zeile 215 eine vergessene Variable eingeschlichen [snip]
Ich meinte glaube ich ne andere Datei, konkret das

Code: Alles auswählen

except:
    raise
Astorek
User
Beiträge: 72
Registriert: Samstag 24. Januar 2009, 15:06
Kontaktdaten:

Hm... Ich habe irgendwo mal aufgeschnappt, dass dies nötig sein soll, wenn man mit try..except arbeitet, damit alle anderen möglicherweise auftretende Fehler weiterhin ihre "Raise"-Meldung anzeigen... :K . Hatte mich auch gewundert, ist aber scheinbar doch nicht (mehr?) nötig...
EyDu
User
Beiträge: 4881
Registriert: Donnerstag 20. Juli 2006, 23:06
Wohnort: Berlin

Astorek hat geschrieben:Hm... Ich habe irgendwo mal aufgeschnappt, dass dies nötig sein soll, wenn man mit try..except arbeitet, damit alle anderen möglicherweise auftretende Fehler weiterhin ihre "Raise"-Meldung anzeigen... :K . Hatte mich auch gewundert, ist aber scheinbar doch nicht (mehr?) nötig...
Das War noch nie notwendig, außer du möchtest vorher noch etwas ausführen:

Code: Alles auswählen

try:
    ...
except ...:
    print "raise notwendig um Meldung auszugeben und Exception danach zu werfen"
    raise

#oder

try:
    ...
except ...:
    #Diesen try-except-Block kannst du dir sparen
    raise
Das Leben ist wie ein Tennisball.
Antworten