Kurz zu Deinen Vorstellungen zu einer GUI: Ja, es gibt einige Projekte, die interessant für Dich sein könnten (FIFE-Engine z.B.), aber nein, *imho* bist Du noch nicht so weit. Du musst noch weiter an den Grundlagen üben und an Deinem Code feilen!
Ich habe mal (wahllos) folgendes Beispiel aus Deinem Code herausgegriffen. Es handelt sich um das Modul ``Klasse_Kampf``:
Code: Alles auswählen
from Klasse_Schiff import neuesSchiff
import random
def summeMatrosen(konvoi):
matrosen = 0
for i in konvoi:
matrosen += i.matrosen
return matrosen
def summeKanonen(konvoi):
kanonen = 0
for i in konvoi:
kanonen += i.lager.fracht.ladung.get('Kanone')
return kanonen
def summeMunition(konvoi):
munition = 0
for i in konvoi:
munition += i.lager.fracht.ladung.get('Kanonenkugel')
return munition
def summeHP(konvoi):
hp = 0
for i in konvoi:
hp += i.hp
return hp
class Kampf():
def __init__(self, angreiferKonvoi, verteidigerKonvoi):
self.aggressor = angreiferKonvoi
self.verteidiger = verteidigerKonvoi
self.kugelschaden = 1.2
def automatisch(self, modus='versenken'):
angreifer_matrosen = summeMatrosen(self.aggressor)
angreifer_kanonen = summeKanonen(self.aggressor)
angreifer_munition = summeMunition(self.aggressor)
angreifer_hp = summeHP(self.aggressor)
verteidiger_matrosen = summeMatrosen(self.verteidiger)
verteidiger_kanonen = summeKanonen(self.verteidiger)
verteidiger_munition = summeMunition(self.verteidiger)
verteidiger_hp = summeHP(self.verteidiger)
if modus == 'versenken':
spieler = random.randint(0,1)
while True:
if spieler == 0:
print('Spieler: Angreifer')
schaden = (0.5 * angreifer_kanonen)* self.kugelschaden
verteidiger_hp -= schaden
print('hat {} Schaden angerichtet - {}'.format(schaden, verteidiger_hp))
input()
if verteidiger_hp <= 0:
print('Angreifer hat gewonnen')
break
spieler = 1
if spieler == 1:
print('Spieler: Verteidiger')
schaden = (0.5 * verteidiger_kanonen) * self.kugelschaden
angreifer_hp -= schaden
print('hat {} Schaden angerichtet - {}'.format(schaden, angreifer_hp))
input()
if angreifer_hp <= 0:
print('Verteidiger hat gewonnen')
break
spieler = 0
- Modulnamen sollten eigentlich eher klein geschrieben werden. Dazu sollte in PEP8 etwas stehen; man kann sich aber mal einfach die
Doku angucken und runterscrollen... da sind alle Module der Standard-Lib klein geschrieben.
- Es fehlen einfach alle üblichen Kommentare! Schau Dir mal
Code von lunar an; da siehst Du, was so alles an Kommentaren in Code gehört (Lizenz, Docstrings). Bei Python nutzt man Restructured Text in den
Document-Strings; die
speziellen Auszeichnungen für Python findest Du hier. Du kannst Sphinx auch erst einmal "weglassen", aber zumindest Doc-Strings solltest Du schreiben und da gehört auch einer für das Modul dazu. Denn nur so erfährt man, was in dem Modul zu finden ist.
- als nächstes fällt auf, dass das, was Du importierst, einen falschen Namen hat. MixedCase gibt es in Python nicht! Lies Dir bitte PEP8 durch und refactor den Code dahingegend. Auch wenn es Mühe macht, es lohnt sich. Insbesondere auch, wenn Du hier im Forum fragst. Solchen inkonformen Code mag sich niemand gerne angucken. Und es kommt zu Unklarheiten - was genau ist ``neuesSchiff``? Eine Klasse? Eine Funktion? Eine Konstante? Iirc gibt es auch ein PEP8-Prüftool, welches einem Verletzungen meldet.. da musste mal googlen. (evtl. kann pyflakes das - das ist ein allgemeiner gefasstes "Stil-Untersuchungstool"). Die PEP8-Verletzungen betreffen dann auch alle Funktionen
- Die ganzen Funktionen haben schlechte Namen. Eine Funktion sollte idR. ein Verb + Objekt sein. Du kombinierst aber zwei Substantive. ``summeMatrosen`` wäre also eigentlich ``matrosen_summieren``. Aber: Das ist letztlich immer noch ein schlechter Name, denn da werden ja keine "Matrosen"-Objekte summiert, sondern aus einem ``Konvoi``-Objekt die Anzahl an Matrosen bestimmt. Ergo würde ich ``zaehle_matrosen_im_konvoi`` vorschlagen. (Ja das ist ziemlich lang; das liegt auch am "Deutsch" für die Namen - das Englische erlaubt oftmals kürzere Konstrukte). Grundsätzlich frage ich mich da aber, wieso ein ``Konvoi``-Objekt nicht selber eine ``zaehle_matrosen``-Methode besitzt? (Fortschrittlicher wäre dafür eine Property zu verwenden - aber das muss als Anfänger nicht sein).
- Die Funktionen sind alle analog - und eher unschön - aufgebaut. ``i`` ist ein schlechter Name für eine Laufvariable. ``i`` nutzt man idR. für Indizes, also etwa bei ``for i in range(...)``-Konstrukten. Da mir auch immer noch unklar ist, was für ein Objekt sich hinter ``Konvoi`` verbirgt, hätte ich jetzt etwa ``schiff`` o.ä. erwartet. Grundsätzlich kann man solche Summationen einfacher als Generator-Expression schreiben:
Code: Alles auswählen
# statt
matrosen = 0
for i in konvoi:
matrosen += i.matrosen
# einfacher
sum(i.matrosen for i in konvoi) # ``i`` nur deswegen, weil ich nicht *weiß*, was man stattdessen nehmen sollte!
- Deine Klasse ist eigentlich keine. Denn sie hat keinen inneren Zustand. Du hättest ``automatisch`` einfach als Funktion schreiben können! Generell ist ``automatisch`` ein Adjektiv und wieder kein Verb; also ein schlechter Name.
- ``automatisch`` verletzt das
DRY-Prinzip! D.h. die Routinen für Angreifer und Verteidiger sind fast identisch; ergo sollte man die Gemeinsame Basis zusammen fassen und generalisieren, z.B. als separate Funktion, die man mit den sich unterscheidenden Merkmalen aufruft.
- Du holst Dir in ``automatisch`` Werte, die Du dann gar nicht mehr benutzt!
- Dieses ``if modus = ...``-Konstrukt lässt vermuten, dass Du noch andere Kampfmöglichkeiten einbauen willst. Dies solltest Du *niemals nie* über solch ein ``if... elif... elif_n... else``-Konstrukt lösen! Zum einen wird solch eine Kaskade unübersichtlich (insbesondere, wenn man neben dem "Dispatching" auch die Funktionalität in dieselbe Funktion schreibt), zum anderen verletzt sie das
Open-Closed-Prinzip. Du kannst so etwas vermeiden, indem Du die Funktionen oder allgemein "Callables" in eine Datenstruktur packst (Liste, Set, Dictionary usw.) und durch einen für Dich passenden Mechanismus auswählst (= Dispacthing). Ein Beispiel, welches das Prinzip an sich aufgreift, findest Du in einem
Tutorial von mir.
Mal vereinfacht könnte das in die Richtung gehen:
Code: Alles auswählen
from random import choice
def versenken(...):
print("versenke Schiff")
def als_prise_nehmen(...):
print("entere Schiff und erobere es")
def in_brand_setzen(...):
print("setze Schiff in Brand")
kampf_optionen = [
versenken,
prise_nehmen,
in_brand_setzen
]
# wählen (Dispatching!) hier per Zufall. Könnte aber auch per Index, Schlüssel usw. funktionieren
gewaehlte_kampf_funktion = choice(kampf_optionen)
# aufrufen
gewaehlte_kampf_funktion(...)
- Wie BlackJack zuletzt schon monierte, ist Deine Aufteilung imho viel zu granular. Man kann und darf in Python wirklich mehr in ein Modul schreiben, als eine Klasse
Imho kann man alles, was an Basistypen bei Dir so rumschwirrt, in *ein* Modul packen. Der Ablauf des Spiels, also Kämpfe und Aktionen usw. könnte man dann ggf. in ein separates Modul stecken.
- Du solltest Dich neben all den hier angeschnittenen Themen wirklich mal mit dem Thema Unit-Testing auseinander setzen. Dein Code ist schon dermaßen komplex, dass Dir ein ordentliches Refactoring jetzt vermutlich schwer fallen wird. Iirc hatte ich Dir dazu irgend wann im Thread mal etwas geschrieben.
Ansonsten: Lass Dich nicht entmutigen! Solange Du Kritik aufnimmst, durchdenkst und als Anstoß zum Lernen siehst, wirst Du Dich auch verbessern. Und wenn nicht, hast Du ja für Dich selber dennoch Deinen Spaß
Ich sage das nur, da hier jetzt wenig "positives" steht - das ist halt bei solchen Code-Reviews normal und hat nichts abwertendes an sich! (Viele User lasten uns Regulars hier immer einen rüden Tonfall an, weil sie genau das nicht begreifen; daher sage ich das heute mal explizit
)
