As a guy who started coding a month ago, I would like an honest review of my version of breakout that I created using pygame.
import pygame
import random
import sys
#level1
global level1
level1 =[
[0,0,0,0,1,1,1,0,0,0,0],
[0,0,0,0,1,1,0,0,0,0,0],
[0,0,1,1,1,1,1,1,0,0,0],
[0,0,0,1,0,1,0,0,0,0,0],
[0,0,0,0,0,0,0,0,0,0,0],
[0,0,0,1,0,0,1,0,0,0,0],
[0,0,0,0,0,0,0,0,0,0,0],
[0,0,0,1,0,1,0,0,0,0,0],
[0,0,0,0,0,0,0,0,0,0,0],
[0,0,0,0,0,0,0,0,0,0,0],
[0,0,0,0,0,0,0,0,0,0,0],
[0,0,0,0,0,0,0,0,0,0,0],
[0,0,0,0,0,0,0,0,0,0,0],
[0,0,0,0,0,0,0,0,0,0,0],
[0,0,0,0,0,0,0,0,0,0,0],
[0,0,0,0,0,0,0,0,0,0,0],
[0,0,0,0,0,0,0,0,0,0,0],
[0,0,0,0,0,0,0,0,0,0,0],
[0,0,0,0,0,0,0,0,0,0,0],
[0,0,0,0,0,0,0,0,0,0,0],
[0,0,0,0,0,0,0,0,0,0,0],
[0,0,0,0,0,0,0,0,0,0,0],
[0,0,0,0,0,0,0,0,0,0,0],
[0,0,0,0,0,0,0,0,0,0,0]
]
def reset_array(array):
for line in array:
for i,num in enumerate(line):
if num == 2:
line[i] = 1
#colour
black = (0,0,0)
white = (255,255,255)
red = (255,0,0)
blue = (0,0,255)
#paddle
class paddle:
def __init__(self,x,y,width,height,vel):
self.x = x
self.y = y
self.width = width
self.height = height
self. vel = vel
Paddle = paddle(350,550,80,20,5)
def draw_paddle():
pygame.draw.rect(screen,red,(Paddle.x,Paddle.y,Paddle.width,Paddle.height))
#ball
class ball:
def __init__(self,x,y,rad,velx,vely):
self.x = x
self.y = y
self.rad = rad
self.velx = velx
self.vely = vely
Ball = ball(30,30,10,2,5)
def ball_move():
Ball.x -= Ball.velx
Ball.y -= Ball.vely
#block
class block:
def __init__(self,x,y,width,height):
self.x = x
self.y = y
self.width = width
self.height = height
Block = block(0,0,75,35)
def draw_block(self,array):
x = self.x
y = self.y
for line in array:
for character in line:
if character == 1:
pygame.draw.rect(screen,white,(x,y,self.width,self.height))
pygame.draw.rect(screen,red,(x,y,self.width,self.height),1)
x += self.width
y += self.height
x = 0
def draw_ball():
pygame.draw.circle(screen,white,(Ball.x,Ball.y), Ball.rad)
#init
pygame.init()
#window
swidth = 800
sheight = 600
screen = pygame.display.set_mode((swidth,sheight))
def main():
#gamestart bool
global gamestart
gamestart = False
#player lives
global player_lives
player_lives = 3
#main loop
running = True
while running:
for event in pygame.event.get():
if event.type == pygame.QUIT:
sys.exit()
if player_lives <= 0:
reset_array(level1)
player_lives = 3
#paddlemove
key = pygame.key.get_pressed()
if key[pygame.K_a]:
Paddle.x -= Paddle.vel
if key[pygame.K_d]:
Paddle.x += Paddle.vel
if Paddle.x + 80 < 0:
Paddle.x += swidth
if Paddle.x >= swidth:
Paddle.x -= swidth
#ballmove
posx = Ball.y
posy = Ball.x
row = posx // 35
column = posy // 75
if level1[row][column] == 1:
level1[row][column] = 2
Ball.vely = -Ball.vely
if gamestart == False:
Paddle.x = 350
Ball.x = Paddle.x + 40
Ball.y = Paddle.y - 10
Ball.velx = 2
Ball.vely = 5
if key[pygame.K_SPACE]:
gamestart = True
if gamestart == True:
ball_move()
if Ball.y <= 0:
Ball.vely = - Ball.vely
if Ball.y >= 600:
gamestart = False
player_lives -= 1
print(player_lives)
if Ball.x <= 0:
Ball.velx = - Ball.velx
if Ball.x >= 800:
Ball.velx = - Ball.velx
#collision
if Paddle.x <= Ball.x <= Paddle.x +44 and Paddle.y == Ball.y:
Ball.vely = - Ball.vely
if Ball.x > Paddle.x:
Ball.velx = Ball.velx
if Ball.x < Paddle.x:
Ball.velx = -Ball.velx
if Paddle.x +45 <= Ball.x <= Paddle.x +80 and Paddle.y == Ball.y:
Ball.vely = - Ball.vely
if Ball.x < Paddle.x:
Ball.velx = - Ball.velx
if Ball.x > Paddle.x:
Ball.velx = Ball.velx
#game end
victory = False
for line in level1:
for col in line:
if all(all(item != 1 for item in item)for item in level1):
victory = True
if victory == True:
print("you win")
break
screen.fill(blue)
draw_paddle()
draw_ball()
draw_block(Block,level1)
pygame.display.flip()
main()
2 Answers 2
#level1
global level1
The comment here is not useful, if you have a variable level1, well... that's level1, right?
The same goes for all your other comments in the code. You should think of comments as a way to describe why you did things that specific way or the meaning of something. If you need them to describe what is happening, most likely you need to refactor something.
Also, as already pointed out in the other answers, this is a global variable, which in this case would not be terrible except that if it's level1, you probably also expect to have level2 and maybe level3, which would make it quite ugly, especially if you hardcode your levels in the module.
You might want to look into loading a text file into a variable. You might have different text files named level1.txt, level2.txt and so on, depending on the level.
class paddle:
def __init__(self,x,y,width,height,vel):
self.x = x
self.y = y
self.width = width
self.height = height
self. vel = vel
Paddle = paddle(350,550,80,20,5)
def draw_paddle():
pygame.draw.rect(screen,red,(Paddle.x,Paddle.y,Paddle.width,Paddle.height))
You have a class paddle (as already noted it should be Paddle), so why not having the draw inside the class?
That way you'd have something like:
class Paddle:
def __init__(self,x,y,width,height,vel):
self.x = x
self.y = y
self.width = width
self.height = height
self. vel = vel
def draw(self):
pygame.draw.rect(screen,red,(self.x, self.y, self.width, self.height))
paddle = Paddle(350,550,80,20,5)
...
paddle.draw()
The same goes for block and ball.
Actually, you could go the extra mile and have a list of objects on which you loop to call the draw and maybe a possible update function.
Something like:
game_objects = [Paddle(350,550,80,20,5), Block(0,0,75,35), Ball(30,30,10,2,5)]
for game_object in game_objects:
game_object.update()
game_object.draw()
As already noted, these magic numbers (e.g. row = posx // 35) are better written as constants, even if you use them only once, to explain their meaning.
if Paddle.x <= Ball.x <= Paddle.x +44 and Paddle.y == Ball.y:
Ball.vely = - Ball.vely
if Ball.x > Paddle.x:
Ball.velx = Ball.velx
if Ball.x < Paddle.x:
Ball.velx = -Ball.velx
if Paddle.x +45 <= Ball.x <= Paddle.x +80 and Paddle.y == Ball.y:
Ball.vely = - Ball.vely
if Ball.x < Paddle.x:
Ball.velx = - Ball.velx
if Ball.x > Paddle.x:
Ball.velx = Ball.velx
This is a block of code that is prone to errors, because of the multiple conditions check and the multiple nested ifs.
You don't need the Ball.velx = Ball.velx line, that doesn't do anything, which mean that you can remove that check.
There's also an issue with checking a strict Paddle.y == Ball.y because if you increase the speed of the ball, there's the chance that it will go past the paddle's y and not be detected, so you should check if the ball's y plus the ball's height is within the paddle's y plus the paddle's height.
The outer if would also be more readable if you put that in a function and called it with the specific parameters, something like:
if paddle_and_ball_collided(paddle, ball):
Ball.vely = - Ball.vely
if Ball.x < Paddle.x:
Ball.velx = -Ball.velx
You should also look into the integrated collision detection functions from pygame, they can save you a bit of trouble (see e.g. https://stackoverflow.com/questions/29640685/how-do-i-detect-collision-in-pygame )
Welcome to CodeReview!
Formatting
Get an IDE such as PyCharm or VSCode that's capable of linting. It will give you several "PEP8" suggestions, such as newlines around functions, that will make your code more legible. Some particulars that will be suggested:
paddle=Paddle(classes)black=BLACK(global constants)Ball=balland vice versa - instance variables should be lowercase, classes uppercase
Avoid globals
They're a messy way of tracking information. In particular, ball and block instances, and the screen instance should be moved to local variables, maybe class members.
Simplify logic
if Paddle.x + 80 < 0:
becomes
if Paddle.x < -80:
Global code
pygame.init()
#window
swidth = 800
sheight = 600
screen = pygame.display.set_mode((swidth,sheight))
Stuff like this should move into your main function.
Long methods
main should be subdivided into several subroutines for legibility and maintainability.
Magic numbers
"44" doesn't mean much to us, and in six months it probably won't mean much to you either. It's best to put this and similar quantities into named constants.
Nested all
if all(all(item != 1 for item in item)for item in level1):
You don't need an inner all. One level of all and two inner comprehension loops will work.
Level data
Your level1 array is a reasonable representation of the level during program runtime. However, it's verbose to construct manually. You'd probably be better off representing this with a more compact string-based format and parsing it out on program load.