I wrote this 2048 game using Python 3 and pygame a few weeks ago. Since I'm getting into CS this summer, I would like to get some review on how I can improve my code in general. Any tips and improvements are welcome, and highly appreciated.
The code is found below:
import pygame
import random
class Main:
def __init__(self):
# SETUP pygame environment
pygame.init()
self.screen = pygame.display.set_mode((500,500))
pygame.display.set_caption('2048')
self.clock = pygame.time.Clock()
# Create the Board
self.board = Board()
#self.board.add_tile()
self.run()
def paint(self):
# Paint the background
self.screen.fill((240,240,206))
# Paint the board object to screen
self.board.paint(self.screen)
pygame.display.update()
# Handling pygame.QUIT event, and
# KEYDOWNS. Keydowns is parsed to board.move function
def eventhandler(self):
for event in pygame.event.get():
if event.type == pygame.QUIT:
self.running = False
pygame.quit()
if event.type == pygame.KEYDOWN:
if event.key == pygame.K_LEFT:
self.board.move("LEFT")
if event.key == pygame.K_RIGHT:
self.board.move("RIGTH")
if event.key == pygame.K_UP:
self.board.move("UP")
if event.key == pygame.K_DOWN:
self.board.move("DOWN")
# Update function.
def update(self):
self.board.update_tiles()
# Main loop. 60 FPS
def run(self):
self.running = True
while self.running:
self.clock.tick(60)
self.eventhandler()
self.update()
self.paint()
class Tile:
def __init__(self,x,y,stage):
# SETUP tiles x,y and stage. Stage is the number it represents, 2,4,8 etc.
self.x = x
self.y = y
self.stage = stage
self.colorlist = [(245,240,255),(237,224,200),(242,177,121),(245,149,99),(246,124,95),(246,94,59),(237,207,113),(237,204,97),(237,200,80),(237,197,63),(238,205,94)]
# Move the tile to new x,y coordinates. Returns False if it moves into a wall.
def move_tile(self,x=0,y=0):
self.x += x
self.y += y
if self.x<0 or self.x > 3 or self.y < 0 or self.y > 3:
self.x -= x
self.y -=y
return False
return True
# Merge two tiles.
def merge(self,Tile):
if Tile.stage == self.stage:
self.increasestage()
return True
else:
return False
def increasestage(self):
self.stage += 1
# Draw the tile to Board.
def draw(self,screen,x,y,font):
pygame.draw.rect(screen,self.colorlist[self.stage-1],(x,y,87,87))
# draw the numbers on tiles:
if self.stage <= 2:
color = (120,110,101)
else:
color = (250,248,239)
text = font.render(str(2**self.stage),2,color)
screen.blit(text,(x+(87/2 - text.get_width()/2), y + (87/2 -text.get_height()/2)))
class Board:
def __init__(self):
## self.tiles keep track of the tiles GUI positions.
self.tiles = [[0,0,0,0] for i in range(4)]
self.board = pygame.Rect(50,50,400,400)
self.color = (186,173,160)
# tilearray stores the tiles as a list. When self.update_tiles is called
# the tiles in tile_array gets updated in self.tiles (the tiles GUI position)
self.tilearray = []
self.add_tile()
self.add_tile()
self.font = pygame.font.SysFont('comicsans',61)
#Draw the board background to screen.
def paint(self,screen):
pygame.draw.rect(screen,self.color,self.board)
self.drawtiles(screen)
# Draw tiles to screen. If no tile, draw empty square.
def drawtiles(self,screen):
for i,array in enumerate(self.tiles):
for j,tile in enumerate(array):
if tile == 0:
pygame.draw.rect(screen,(204,193,180),(60+i*87+10*i,60+j*87+10*j,87,87))
else:
tile.draw(screen,60+i*87+10*i,60+j*87+10*j,self.font)
# Returns an arraylist with positions in self.tiles which are empty
def get_empty_spaces(self):
empty = []
for i,array in enumerate(self.tiles):
for j,tile in enumerate(array):
if tile==0:
empty.append([i,j])
return empty
# Add a new tile to the game. Coordinates chosen at random.
def add_tile(self):
empty = self.get_empty_spaces()
chosen = random.choice(empty)
if random.randrange(1,100) <10:
stage = 2
else:
stage = 1
t = Tile(chosen[0],chosen[1],stage)
self.tilearray.append(t)
# Move all tiles on the board.
def move(self,key):
stepstaken = 0
if key=="LEFT":
for i, array in enumerate(self.tiles):
for j, _ in enumerate(array):
tile = self.tiles[j][i]
if tile!=0:
stepstaken += self.move_single_tile(tile,-1,0)
self.update_tiles()
if key =="RIGTH":
for i,array in enumerate(self.tiles):
for j,_ in enumerate(array):
tile = self.tiles[3-j][3-i]
if tile!= 0:
stepstaken += self.move_single_tile(tile,1,0)
self.update_tiles()
if key == "UP":
for i,array in enumerate(self.tiles):
for j,_ in enumerate(array):
tile = self.tiles[i][j]
if tile!=0:
stepstaken += self.move_single_tile(tile,0,-1)
self.update_tiles()
if key == "DOWN":
for i, array in enumerate(self.tiles):
for j,_ in enumerate(array):
tile = self.tiles[3-i][3-j]
if tile!=0:
stepstaken += self.move_single_tile(tile,0,1)
self.update_tiles()
if stepstaken>0:
self.add_tile()
# Tiles are stored in self.tilearray. When updating, the tiles from self.tilearray is
# stored in the 2d array.
def move_single_tile(self,tile,vx=0,vy=0):
steps = 0
for i in range(0,3):
if self.position_is_inside_grid(tile.x+vx,tile.y+vy) and self.tile_is_empty(tile.x+vx,tile.y+vy):
tile.move_tile(vx,vy)
steps+=1
else:
if self.position_is_inside_grid(tile.x+vx,tile.y+vy) and self.tiles[tile.x+vx][tile.y+vy].merge(tile):
self.tilearray.remove(tile)
steps += 1
return steps
def position_is_inside_grid(self,x,y):
if x>-1 and x<4 and y>-1 and y<4:
return True
else:
return False
def tile_is_empty(self,x,y):
if self.tiles[x][y] == 0:
return True
else:
return False
def update_tiles(self):
self.tiles = [[0,0,0,0] for i in range(4)]
for tile in self.tilearray:
self.tiles[tile.x][tile.y] = tile
m = Main()
# TO DO
# Refactor move() to loop functions
#
#
#
2 Answers 2
The code looks good, but the
if key=="LEFT":
for i, array in enumerate(self.tiles):
for j, _ in enumerate(array):
tile = self.tiles[j][i]
if tile!=0:
stepstaken += self.move_single_tile(tile,-1,0)
self.update_tiles()
if key =="RIGTH":
for i,array in enumerate(self.tiles):
for j,_ in enumerate(array):
tile = self.tiles[3-j][3-i]
if tile!= 0:
stepstaken += self.move_single_tile(tile,1,0)
self.update_tiles()
has some redundant loops. You can move the if
statement so the array looping is only written once.
for i, array in enumerate(self.tiles):
for j, _ in enumerate(array):
if key=="LEFT":
tile = self.tiles[j][i]
if tile!=0:
stepstaken += self.move_single_tile(tile,-1,0)
self.update_tiles()
if key =="RIGTH":
tile = self.tiles[3-j][3-i]
if tile!= 0:
stepstaken += self.move_single_tile(tile,1,0)
self.update_tiles()
I also recommend to use Enums for the directions.
When you replace the
m = Main()
with
if __name__ == "__main__":
m = Main();
This allows you to import this file for example to use the Board
class for another project. Otherwise, the whole game would run on import.
Efficient
In the the eventhandler
function, there are multiple if
statements.
In your code, they are separate if
statements,
which would imply they are unrelated to each other. However, that is not
the case. Since they are related, you should use if/elif
. That better
conveys the intent of the code.
if event.key == pygame.K_LEFT:
self.board.move("LEFT")
elif event.key == pygame.K_RIGHT:
self.board.move("RIGTH")
elif event.key == pygame.K_UP:
self.board.move("UP")
elif event.key == pygame.K_DOWN:
self.board.move("DOWN")
Also, when your code is run, every if
condition is evaluated,
whereas with if/elif
, only the conditions which are needed are
evaluated.
The move
function as has a similar situation when you should use elif
:
if key=="LEFT":
...
elif key =="RIGTH":
User interface
As an added feature, you could show the current score while the game is being played. This is common for the 2048 game.
Also, you should display a message when no more moves are available to indicate the end of the game.
Comments
The following comment is not needed because it is obvious from the function name what the function does:
# Update function.
def update(self):
The following comments should be removed:
# TO DO
# Refactor move() to loop functions
You can keep track of future enhancements elsewhere, such as in another plain text file.
Naming
The PEP 8 style guide recommends using
snake case for functions. For example, eventhandler
would be easier
to understand as event_handler
.
The same is true for increasestage
. In fact, since this function only
increases a single variable, I don't think you really need this function.
Main
is not convey much meaning as a name for a class.
Spelling
RIGHT
is misspelled as RIGTH
.