Verbesserungsvorschläge

Wenn du dir nicht sicher bist, in welchem der anderen Foren du die Frage stellen sollst, dann bist du hier im Forum für allgemeine Fragen sicher richtig.
Antworten
krinust
User
Beiträge: 12
Registriert: Dienstag 25. Oktober 2016, 18:18

Ich versuche mich gerade in PyGame einzuarbeiten und habe dazu den folgenden Code im Internet gefunden:

Code: Alles auswählen

import pygame, sys, random
from pygame.locals import *

#constants representing colours
BLACK = (0,   0,   0  )
BROWN = (153, 76,  0  )
GREEN = (0,   255, 0  )
BLUE  = (0,   0,   255)
WHITE = (255, 255, 255)

#constants representing the different resources
DIRT  = 0
GRASS = 1
WATER = 2
COAL  = 3

#a dictionary linking resources to textures
textures =   {
                DIRT   : pygame.image.load('dirt.png'),
                GRASS  : pygame.image.load('grass.png'),
                WATER  : pygame.image.load('water.png'),
                COAL   : pygame.image.load('coal.png'),
            }

inventory =   {
                DIRT   : 0,
                GRASS  : 0,
                WATER  : 0,
                COAL   : 0
            }

#useful game dimensions
TILESIZE  = 20
MAPWIDTH  = 30
MAPHEIGHT = 20

#the player image
PLAYER = pygame.image.load('player.png')
#the position of the player [x,y]
playerPos = [0,0]

#a list of resources
resources = [DIRT,GRASS,WATER,COAL]
#use list comprehension to create our tilemap
tilemap = [ [DIRT for w in range(MAPWIDTH)] for h in range(MAPHEIGHT) ] 

#set up the display
pygame.init()
DISPLAYSURF = pygame.display.set_mode((MAPWIDTH*TILESIZE, MAPHEIGHT*TILESIZE + 50))

#add a font for our inventory
INVFONT = pygame.font.Font('FreeSansBold.ttf', 18)

#loop through each row
for rw in range(MAPHEIGHT):
    #loop through each column in that row
    for cl in range(MAPWIDTH):
        #pick a random number between 0 and 15
        randomNumber = random.randint(0,15)
        #if a zero, then the tile is coal
        if randomNumber == 0:
            tile = COAL
        #water if the random number is a 1 or a 2
        elif randomNumber == 1 or randomNumber == 2:
            tile = WATER
        elif randomNumber >= 3 and randomNumber <= 7:
            tile = GRASS
        else:
            tile = DIRT
        #set the position in the tilemap to the randomly chosen tile
        tilemap[rw][cl] = tile

while True:

    #get all the user events
    for event in pygame.event.get():
        #if the user wants to quit
        if event.type == QUIT:
            #and the game and close the window
            pygame.quit()
            sys.exit()
        #if a key is pressed
        elif event.type == KEYDOWN:
            #if the right arrow is pressed
            if event.key == K_RIGHT and playerPos[0] < MAPWIDTH - 1:
                #change the player's x position
                playerPos[0] += 1
            if event.key == K_LEFT and playerPos[0] > 0:
                #change the player's x position
                playerPos[0] -= 1
            if event.key == K_UP and playerPos[1] > 0:
                #change the player's x position
                playerPos[1] -= 1
            if event.key == K_DOWN and playerPos[1] < MAPHEIGHT -1:
                #change the player's x position
                playerPos[1] += 1
            if event.key == K_SPACE:
                #what resource is the player standing on?
                currentTile = tilemap[playerPos[1]][playerPos[0]]
                #player now has 1 more of this resource
                inventory[currentTile] += 1
                #the player is now standing on dirt
                tilemap[playerPos[1]][playerPos[0]] = DIRT

            #placing dirt
            if (event.key == K_1):
                #get the tile to swap with the dirt
                currentTile = tilemap[playerPos[1]][playerPos[0]]
                #if we have dirt in our inventory
                if inventory[DIRT] > 0:
                    #remove one dirt and place it
                    inventory[DIRT] -= 1
                    tilemap[playerPos[1]][playerPos[0]] = DIRT
                    #swap the item that was there before
                    inventory[currentTile] += 1

    #loop through each row
    for row in range(MAPHEIGHT):
        #loop through each column in the row
        for column in range(MAPWIDTH):
            #draw the resource at that position in the tilemap, using the correct image
            DISPLAYSURF.blit(textures[tilemap[row][column]], (column*TILESIZE,row*TILESIZE))

    #display the player at the correct position 
    DISPLAYSURF.blit(PLAYER,(playerPos[0]*TILESIZE,playerPos[1]*TILESIZE))

    #display the inventory, starting 10 pixels in
    placePosition = 10
    for item in resources:
        #add the image
        DISPLAYSURF.blit(textures[item],(placePosition,MAPHEIGHT*TILESIZE+20))
        placePosition += 30
        #add the text showing the amount in the inventory
        textObj = INVFONT.render(str(inventory[item]), True, WHITE, BLACK)
        DISPLAYSURF.blit(textObj,(placePosition,MAPHEIGHT*TILESIZE+20)) 
        placePosition += 50

    #update the display
    pygame.display.update()
Quelle: http://usingpython.com/pygame/
Ich würde gerne eure Meinung dazu hören, da ich mir unsicher bin, ob der Autor einen guten Programmierstil einhält. Insbesondere wundert mich, dass es keine main-Funktion gibt und so viele Konstanten definiert werden :?
Zuletzt geändert von Anonymous am Sonntag 13. August 2017, 19:47, insgesamt 1-mal geändert.
Grund: Quelltext in Python-Codebox-Tags gesetzt.
BlackJack

@krinust: Das würde ich nicht als Vorbild nehmen. Und das ist ja noch nicht einmal das Endprodukt — der packt da ja *noch mehr Code* rein. Und auch echt fragwürdige Sachen wie das `controls`-Wörterbuch das Ressourcen auf magische Zahlen abbildet, wofür Pygame ja eigentlich Konstanten hat. Die er vorher auch verwendet hat, nur da dann nicht mehr‽ Und *benutzt* wird das Wörterbuch dann um linear durch die Einträge zu iterieren um zu einer Zahl die Ressource zu bekommen, also eigentlich der umgekehrte Weg. Der Autor hat anscheinend den Sinn von Wörterbüchern nicht verstanden.

Das ist insgesamt viel zu viel unstrukturierter Code. Der sich nicht an die Namenskonventionen hält. Manche Konstanten werden nicht mit Grossbuchstaben benannt, dafür gibt es Werte die verändert werden, deren Namen wie Konstanten geschrieben werden. Für gleiche Dinge werden unterschiedliche Namen verwendet — mal `rw` mal `row`, mal `cl` mal `column`.

Die Karte wird erst mit Dummywerten erstellt, die dann kurz danach alle noch mal durch zufällige Kacheln ersetzt werden, statt gleich die Karte mit den richtigen Werten zu erstellen.

Statt der magischen Zahlen für die Ressourcen hätte man Zeichenketten nehmen können, damit Ausgaben von den Werten auch verständlich sind.

Ich weiss es ist für Anfänger, aber die Kommentare sind *echt* übertrieben. Eigentlich sollte man nur Kommentieren was jemand der Sprache und verwendete Bibliotheken *kennt*, trotzdem nicht, oder falsch verstehen könnte. Aber so etwas wie ``#constants representing colours`` bevor `BLACK`, `WHILE`, `BLUE`, … definiert werden, oder

Code: Alles auswählen

    #loop through each row
    for row in range(MAPHEIGHT):
        #loop through each column in the row
        for column in range(MAPWIDTH):
krinust
User
Beiträge: 12
Registriert: Dienstag 25. Oktober 2016, 18:18

Ich habe mich mal dran versucht:

Code: Alles auswählen

# Introduction to pygame
# http://usingpython.com/pygame-intro/

import pygame, sys
from pygame.locals import *
import random

pygame.init()
tile_groesse = 40
displayw = 10
displayh = 10

fenster = pygame.display.set_mode(
		(displayw*tile_groesse, displayh*tile_groesse)
)

textures = {
	"Gras" : pygame.image.load("gras.png"),
	"Erde" : pygame.image.load("erde.jpg"),
	"Wasser": pygame.image.load("wasser.jpg"),
	"Kohle" : pygame.image.load("kohle.jpg"),
	"Stein" : pygame.image.load("stein.png"),
}


class Spieler(object):
	def __init__(self, x, y, bild):
		self.x = x
		self.y = y 
		self.bild = bild

	def zeichnen(self):
		fenster.blit(self.bild, (self.x*tile_groesse, self.y*tile_groesse))

	def bewegen(self, richtung):
		if richtung == "N":
			self.y -= 1
		if richtung == "S":
			self.y += 1
		if richtung == "O":
			self.x += 1
		if richtung == "W":
			self.x -= 1
	

class MainRun(object):
	def __init__(self, displayw, displayh):
		self.dw = displayw
		self.dh = displayh
		self.Main()

	def Main(self):
		spieler_bild = pygame.image.load("spieler.png").convert_alpha()
		spieler = Spieler(0, 0, spieler_bild)

		tilemap = self.karte_aufbauen()

		while True:
			for event in pygame.event.get():
				if event.type == QUIT:
					pygame.quit()
					sys.exit()
				elif event.type == KEYDOWN: 
					if event.key == K_RIGHT and spieler.x < self.dw -1:
						spieler.bewegen("O")
					if event.key == K_LEFT and spieler.x > 0:
						spieler.bewegen("W")
					if event.key == K_UP and spieler.y > 0:
						spieler.bewegen("N")
					if event.key == K_DOWN and spieler.y < self.dh -1:
						spieler.bewegen("S")
	
			for zeile in range(self.dh):
				for spalte in range(self.dw):
					fenster.blit(
						textures[tilemap[zeile][spalte]], 
						(spalte*tile_groesse, zeile*tile_groesse)
					)

			spieler.zeichnen()
			pygame.display.update()


	def karte_aufbauen(self):
		tilemap = [["Erde" for w in range(self.dw)] for h in range(self.dh)]
		for x in range(self.dh):
			for y in range(self.dw):
				zufallszahl = random.randint(0,15)
				if zufallszahl == 0:
					tile = "Kohle"
				elif zufallszahl == 1 or zufallszahl == 2:
					tile = "Wasser"
				elif zufallszahl >= 3 and zufallszahl <= 7:
					tile = "Gras" 
				else:
					tile = "Erde" 
				tilemap[x][y] = tile
		return tilemap


def main():
	MainRun(displayw, displayh)


if __name__ == '__main__':
	main()
Ist das so besser?
Zuletzt geändert von Anonymous am Dienstag 15. August 2017, 18:37, insgesamt 1-mal geändert.
Grund: Quelltext in Python-Codebox-Tags gesetzt.
krinust
User
Beiträge: 12
Registriert: Dienstag 25. Oktober 2016, 18:18

Bietet sich diese Struktur immer an? Also kann ich jedes PyGame-Spiel prinzipiell so aufbauen (mit der Klasse MainRun)?
BlackJack

@krinust: Die Klasse bietet sich so nie an, das ist nicht wirklich eine Klasse.
krinust
User
Beiträge: 12
Registriert: Dienstag 25. Oktober 2016, 18:18

Ich habe mich an deinen Ratschlägen und an https://stackoverflow.com/questions/356 ... -in-pygame orientiert. Vielleicht könntest du einige Tipps geben, wie ich das Programm verbessern kann.
BlackJack

@krinust: Die Konstanten sind klein geschrieben und es gibt immer noch eine Variable auf Modulebene: `fenster`. Und `pygame.init()` gehört auch nicht auf Modulebene. Steht da ja auch nur weil `fenster` das braucht.

Wenn fenster nicht mehr global ist, kann `Spieler.zeichnen()` da auch nicht mehr einfach so drauf zugreifen, sondern muss das als Argument bekommen.

`MainRun` macht als Klasse keinen Sinn. Klassen sind Vorlagen um Objekte zu erstellen die Daten und Funktionen die auf diesen Daten operieren, sinnvoll zusammen zu fassen. Der Aufruf von `MainRun()` liefert dem Aufrufer aber gar kein Objekt, sondern kehrt gar nicht zurück, weil die `__init__()` die `Main()`-Methode aufruft, die das Hauptprogramm enthält. Selbst wenn dem nicht so wäre, ist die `MainRun`-Klasse nicht sinnvoll. Das lässt sich mit Funktionen viel weniger kompliziert ausdrücken. Und dann macht die Aufteilung des Codes auf `main()`, und den Funktionen die aus `MainRun.__init__()` und `MainRun.Main()` entstehen, keinen Sinn, das kann also alles erst einmal in die `main()`-Funktion.

`tile_groesse` ist Denglish. Entweder `kachel_groesse` oder `tile_size`. Ich würde komplett Englisch empfehlen.

`displayw` und `displayh` sind falsch und kryptisch. Falsch weil es gar nicht Breite und Höhe des Display-Surface ist, sondern der Karte in Kacheln. Und kryptisch weil man raten muss was `h` und `w` heisst. Zu `dw` und `dh` braucht man dann glaube ich nichts mehr zu sagen.

Beim Aufbauen der Karte wird immer noch die komplette Datenstruktur mit ”Wegwerfwerten” erstellt.

Die Bedingungen dort sind auch ein bisschen, ähm, vielfältig. Mal mit ``and``, mal mit ``or``, wo eigentlich immer *ein* ``<=`` reichen würde, denn das die Zahl grösser als im vorherigen Zweig war ist ja bereits klar.

Bei der `Spieler`-Klasse sollte man aufpassen das man das Rad nicht neu erfindet, also in diesem Fall `pygame.sprite.Sprite`.

Zwischenstand (volkommen ungetestet):

Code: Alles auswählen

import random
import pygame
from pygame.locals import *


TILE_SIZE = 40
MAP_WIDTH = 10
MAP_HEIGHT = 10
TEXTURES = {
    'Gras': pygame.image.load('gras.png'),
    'Erde': pygame.image.load('erde.jpg'),
    'Wasser': pygame.image.load('wasser.jpg'),
    'Kohle': pygame.image.load('kohle.jpg'),
    'Stein': pygame.image.load('stein.png'),
}


class Spieler(object):
    def __init__(self, x, y, bild):
        self.x = x
        self.y = y
        self.bild = bild
 
    def zeichnen(self, fenster):
        fenster.blit(self.bild, (self.x * TILE_SIZE, self.y * TILE_SIZE))
 
    def bewegen(self, richtung):
        if richtung == 'N':
            self.y -= 1
        if richtung == 'S':
            self.y += 1
        if richtung == 'O':
            self.x += 1
        if richtung == 'W':
            self.x -= 1
   

def get_random_tile():
    zufallszahl = random.randint(0, 15)
    if zufallszahl <= 0:
        return 'Kohle'
    elif zufallszahl <= 2:
        return 'Wasser'
    elif zufallszahl <= 7:
        return 'Gras'
    return 'Erde'


def karte_aufbauen(width, height):
    return [[get_random_tile() for _ in range(width)] for _ in range(height)]


def main():
    pygame.init()
    fenster = pygame.display.set_mode(
        (MAP_WIDTH * TILE_SIZE, MAP_HEIGHT * TILE_SIZE)
    )
    spieler_bild = pygame.image.load('spieler.png').convert_alpha()
    spieler = Spieler(0, 0, spieler_bild)
    tile_map = karte_aufbauen(MAP_WIDTH, MAP_HEIGHT)
    try:
        while True:
            for event in pygame.event.get():
                if event.type == QUIT:
                    return
                elif event.type == KEYDOWN:
                    if event.key == K_RIGHT and spieler.x < MAP_WIDTH - 1:
                        spieler.bewegen('O')
                    if event.key == K_LEFT and spieler.x > 0:
                        spieler.bewegen('W')
                    if event.key == K_UP and spieler.y > 0:
                        spieler.bewegen('N')
                    if event.key == K_DOWN and spieler.y < MAP_HEIGHT - 1:
                        spieler.bewegen('S')

            for zeile in range(MAP_HEIGHT):
                for spalte in range(MAP_WIDTH):
                    fenster.blit(
                        TEXTURES[tile_map[zeile][spalte]],
                        (spalte * TILE_SIZE, zeile * TILE_SIZE)
                    )

            spieler.zeichnen(fenster)
            pygame.display.update()
    finally:
        pygame.quit()
 
 
if __name__ == '__main__':
    main()
Antworten