hackfleisch89ru - ein OpenSource 2D shooter

Stellt hier eure Projekte vor.
Internetseiten, Skripte, und alles andere bzgl. Python.
Benutzeravatar
str1442
User
Beiträge: 520
Registriert: Samstag 31. Mai 2008, 21:13

Beitragvon str1442 » Sonntag 11. Januar 2009, 01:34

Jetzt wollte ich doch tatsächlich die Spielbedingungen etwas anpassen und hab die <game>.player.hp's auf 1000 hochgesetzt und siehe da:

Traceback (most recent call last):
File "main.py", line 127, in <module>
g.draw(screen)
File "/media/Daten/Projects/hackfleisch89ru/hackfleisch89ru/pyage.py", line 1287, in draw
self.draw_player_panel(dest)
File "/media/Daten/Projects/hackfleisch89ru/hackfleisch89ru/pyage.py", line 1275, in draw_player_panel
surf = self.font.render(render_string, True, player_panel_color)
TypeError: Invalid foreground RGBA argument


Cheater's always get their Punishment? :D

Also rein äusserlich super! Zum Code:

1. Der Traceback: Du benutzt an der Stelle im Code die hp Anzahl um die Farbe zu berechnen. Wo ist da die Logik? Das hat doch gar nichts miteinander zu tun.

2. Du hast keinen einzigen Docstring benutzt. Unglaublich praktische Dinger das sind.

3. Dein Code ist sehr zentralistisch angelegt. Du machst bis auf 4 Callback Funktionen absolut alles in den Klassen, von Datei laden bis "eigenes Animationsformat parsen". Du hast auch viele lose Methoden an die Klassen gebunden, die sogar sehr allgemein gehalten sind (load_image). Die Klassen sind total überladen und haben zig Methoden, "game" ist ein richtiges Gottobjekt. Die Klassen kennen sich untereinander sehr genau und benutzen sich in den unterschiedlichsten Teilen des Codes. Wenn man Klassen mehr als einmal initialisieren muss sollte man imho eine klassenglobale Referenz erstellen anstatt jedesmal direkt drauf zu referieren oder den entsprechenden Code in allgemeine Funktionen auslagern.

4. Überall irgendwelche Zahlen und Rechenoperationen, Strings mit denen man auf scheinbar wichtige Zustände und Objekte referiert. Sollte man in globale Konstanten umwandeln und solche kleineren Rechnereien auf das Minimum beschränken und nicht unkommentiert stehen lassen. Auch greifst du einfach mal über den Index auf eine Sequenz zu, die du aus einer Datei geholt hast. Kann man natürlich machen, aber solche hardkodierten Werte vermeidet man besser oder sagt was dazu. (Es sei denn man nutzt einen Tupel als "Struct", sowas sollte aber transparent sein - Listen würde ich prinzipiell nur mit dynamisch generierten Index Werten behandeln, wenn überhaupt)

5. Generell Duplikation. Da ist dein Code schon deutlich besser im Gegensatz zu dem, was ich schon gesehen habe, aber es sind immernoch einige drin. Zum Beispiel die Rechnenoperationen. Jede Ecke an der ein längerer Codeteil sitzt zusammen mit if / elif's oder auch allein riecht schon nach besserer Aufteilung.

6. Du benutzt Strings um auf Klassen in Dicts zuzugreifen. Python ist eine sehr dynamische Sprache, du kannst die Klassen direkt referieren. Du kannst in Python absolut jedes Objekt (und alles ist ein Objekt) herumreichen, irgendwo eintragen, in eine Liste stecken und darüber gemeinsam iterieren etc. (Außer natürlich die eines der entsprechenden Objekte erlaubt das nicht; Also, daß man beispielsweise in Dict's keine Listen als Keys verwenden kann ist so eine Einschränkung, die hat aber auch einen Grund).

7. Deine Klassen sind alle Oldstyle Klassen.

8. Ein paar Kommentare mehr schaden nie.

9. Trennung von Logik und GUI ist nicht gegeben. Du hast wie gesagt auch eine sehr starke Bindung zwischen den einzelnen Komponenten. Kapselung einzelner Verantwortlichkeiten nur durch Methoden.

10. Du hast an manchen Stellen komplett toten Code rumliegen, was sich in trivialen Syntaxfehlern äussert. In Zeile 1222 steht zum Beispiel "vec,y".

11. Die Exceptions, die du abfängts, lassen die Methoden frühzeitig zurückkehren oder setzen einen Parameter auf None. Was soll das? Wenn du eine Exception nicht sinnvoll behandeln kannst, dann lass sie durch und schreib einen Kommentar in den Code, der erläutert warum. Wenn du in load_image() einen Fehler hast, wird self.image auf None gesetzt und dein rect bekommt nichtmal einen ordentlichen size Parameter. Wenn es dann 500 Meter weiter deswegen kracht, bekommst du einen AttributeError auf None oder schlimmer, einen Fehler wegen einer falschen Rect Size, und du wunderst dich erstmal woher das nun kommt.

12: Den Code in main würde ich in eine main() Funktion stecken. Dann wird der Code nicht direkt ausgeführt und du hast einen einheitlichen Startpunkt. Moduleebenen Code ist meist unschön. Damit bei einem direkten Start main() ausgeführt wird, trage das am Ende von main.py ein:

Code: Alles auswählen

if __name__ == "__main__":
    main()


__main__ ist der Name eines Modules, das direkt gestartet wird.

13. Du solltest Pygame Images mit convert() konvertieren, sonst wird immer die PNG Datei angesprochen. Dadurch wird dein Code nur langsamer.

14. Du benutzt pygame.display.flip(). Damit wird dein Display in jedem Durchlauf der Schleife komplett geupdatet. Das ist natürlich langsam. Schau dir mal auf pygame.org unter Tutorials die 12 Tipps oder wie es hieß an. Du kannst dir auch mal meinen Tetris Klon Tetrix0r anschauen, da arbeitet ich mit einer Liste, die die zu aktualisierenden Rect's enthält. (Der Code ist aber schon älter und nicht der schönste; findest du hier im Forum).

15. Warum benutzt du die Event Schleife und xyz.get_pressed()?

So, das wäre das gröbste - Schlussendlich mein Rat: PEP8 lesen, import this ausführen und Single Responsibility beachten ^_^

Achja; schau dir mal das Tool Pylint an. Das zeigt dir sehr viele, vor allem stylistische, Problematiken automatisch an. Sollte dir helfen. Aber Achtung, du wirst bei deiner pyage.py fast erschlagen vor Meldungen.
SyrischerRaubelefant
User
Beiträge: 14
Registriert: Sonntag 7. Januar 2007, 20:59

Beitragvon SyrischerRaubelefant » Sonntag 11. Januar 2009, 02:06

Also erstmal danke für alle Tips :)

Du benutzt an der Stelle im Code die hp Anzahl um die Farbe zu berechnen. Wo ist da die Logik? Das hat doch gar nichts miteinander zu tun.


Die HP-Anzeige oben links im Bild ändert ihre (Vordergrund-) Farbe je nach HP Anzahl.
Bei vollem HP ist die Schrift weiß. Je mehr man getroffen wird, also je weniger HP man hat, desto "roter" wird die Schrift, bis es schließlich blutrot bei "HP = 0 = tot" wird. Damit wollte ich dem user die Möglichkeit geben, frühzeitig gewarnt zu werden, wenn die HP niedrig ist ohne dass der user direkt auf die HP-Anzeige schauen muss. (Falls man mit den Gegnern beschäftigt ist und gerade nicht von einer HP-Zahl abgelenkt werden will, fällt einem das rote im oberen Bildbereich gewiss auf)

Woher der Fehler jetzt herkommt, muss ich mir später genauer anschauen, bin grad zu müde.
Besuch uns im euIRC:
server: irc.euirc.net
channel: #alle_in_den_chat
Benutzeravatar
str1442
User
Beiträge: 520
Registriert: Samstag 31. Mai 2008, 21:13

Beitragvon str1442 » Sonntag 11. Januar 2009, 14:26

Das sollte man aber nicht direkt über die hp Anzahl regeln, sonst wird sowas total untransparent. Du könntest zum Beispiel einen Tupel anlegen, der die einzelnen Untertupel mit den Farben enthält. Dann berechnest du einfach "round(start_hp / jetzige_hp)" und benutzt den herauskommenden Wert als Index für den Tupel und Ende. Für solche Zwecke sind Tupel mitunter auch gedacht, als eine Art "struct". Hast du Beispielsweise nur noch 200 HP, kommt der Wert 3 raus -> Sollte dann natürlich schon recht rot sein.

Werde übringens eine Woche vermutlich nicht antworten können, nur damit du dich nicht wunderst.

EDIT: Gut, strebt so natürlich etwas stark gegen große Zahlen, insofern müsstest du das noch etwas einschränken oder einfach auf 25 % usw überprüfen, aber war der erste Gedanke.

Wer ist online?

Mitglieder in diesem Forum: 0 Mitglieder