4
\$\begingroup\$

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
#
#
#
toolic
14.5k5 gold badges29 silver badges203 bronze badges
asked May 27, 2019 at 11:13
\$\endgroup\$

2 Answers 2

4
\$\begingroup\$

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.

toolic
14.5k5 gold badges29 silver badges203 bronze badges
answered May 29, 2019 at 13:01
\$\endgroup\$
1
\$\begingroup\$

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.

answered Aug 8, 2024 at 13:34
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.