This is the first time that my code is being reviewed after two years of learning of Python as autodidact. I've never took a course or a class, but I've only read books and seen some videos on youtube.
I've rewritten from scratch the 2048 game, trying to make the code more readable and performant possible.
Cell.py
from uuid import uuid4
from collections import defaultdict
WAIT = 1 # ms between frames
COLOR_TABLE = defaultdict(lambda: "#ffffff",
**{"2": "#f4e242", "4": "#f4b841", "8": "#f49d41", "16": "#f48241",
"32": "#f46741", "64": "#f45541", "128": "#f44141", "256": "#f4416d"})
def sign(n):
""" Return the sign of a number """
if n > 0:
return 1
elif n < 0:
return -1
return 0
class Cell:
def __init__(self, canvas, root, pos, l, n=2):
self.canvas = canvas
self.root = root
self.n = n # Number to display
self.pos = tuple(pos) # Position on the table as (x, y)
self.l = l # Side leght
self.font = ("Helvetica", int(self.l / 3))
self.id = str(uuid4()) # Personal id of every cell
self._draw()
def move(self, x, y):
""" Function called by the user to move the cell of (x, y) position (the grid is 4 positions wide """
if x != 0 and y != 0:
return # It can't move diagonally
self._moveloop(x * self.l, y * self.l)
def double(self):
self.n *= 2
self.canvas.itemconfig(self.id + "text", text=self.n)
self.canvas.itemconfig(self.id + "cell", fill=COLOR_TABLE[str(self.n)])
def _draw(self):
""" Draws the cell and his number on the canvas"""
x, y = self.pos
self.canvas.create_rectangle(x * self.l, y * self.l, (x + 1) * self.l, (y + 1) * self.l, fill=COLOR_TABLE[str(self.n)],
tag=(self.id, self.id + "cell"))
self.canvas.create_text(x * self.l + (self.l / 2), y * self.l + (self.l / 2), text=self.n, font=self.font, tag=(self.id, self.id + "text"))
def _moveloop(self, tomovex, tomovey):
""" Recursive function that moves the cell 1px each call """
if not tomovex and not tomovey:
return # Break the loop
self.canvas.move(self.id, sign(tomovex), sign(tomovey))
newx = (abs(tomovex) - 1) * sign(tomovex)
newy = (abs(tomovey) - 1) * sign(tomovey)
self.root.after(WAIT, lambda: self._moveloop(newx, newy))
def __del__(self):
""" When the cell is overwritten his canvas elements must be deleted """
self.canvas.tag_lower(self.id)
self.root.after(int(self.l * 4), lambda: self.canvas.delete(self.id))
def __repr__(self):
""" Debug purpose """
return "C(%s)" % self.n
Main.py
from tkinter import *
from random import randint as ri
from copy import copy
import cell
WIDTH = 400
class App:
def __init__(self, parent):
self.root = parent
# Array where all the cells are saved
self.table = [0] * 16
# Boolean to control user inputs to avoid too many clicks
self._canclick = True
# Score
self._score = 0
# Draws all the tkinter elements
self.main_canvas = Canvas(self.root, width=WIDTH, height=WIDTH, bg="lightblue")
self.main_canvas.pack()
self.second_frame = Frame(self.root)
self.second_frame.pack()
self._scorevar = StringVar()
self._scorevar.set(self._score)
self._sframesetup()
# Draws the horizontal and vertical lines
self._griddraw()
# Draws the cells
self._cellsetup(3)
def callback(self, direction):
""" Handles the user input
direction: LEFT, RIGHT, DOWN, UP = 0, 1, 2, 3"""
if self._canclick:
self._canclick = False # Blocks the user input
# Makes a copy of the table to check later if something changed or not
backup = copy(self.table)
d = dict(enumerate([self._left, self._right, self._down, self._up]))
d[direction]() # Calls the right movement function
# Check if there is space to spawn a new cell
if not 0 in self.table:
self._lose()
return
if backup != self.table:
# Waits until the cells stop and spawns a new one
self.root.after(301, self._spawnnew)
else:
self._canclick = True
def restart(self):
""" Restart button callback """
# deletes lose text
self.main_canvas.delete("d72819d9")
# deletes all cells
self.table = [0] * 16
self._cellsetup(3)
self._scorevar.set(0)
def _sframesetup(self):
pointframe = Frame(self.second_frame)
pointframe.pack(side=LEFT, pady=20, padx=20)
Label(pointframe, text="Punteggio:").pack(side=LEFT)
Label(pointframe, textvariable=self._scorevar).pack(side=LEFT)
restartb = Button(self.second_frame, text="Restart", command=self.restart)
restartb.pack(side=RIGHT, pady=20, padx=20)
def _griddraw(self):
""" Draws the horizontal and vertical lines """
line_color = "blue"
cell_width = WIDTH / 4
for n in range(1, 4):
# Vertical lines
self.main_canvas.create_line(n * cell_width, 0, n * cell_width, WIDTH, fill=line_color)
# Horizontal lines
self.main_canvas.create_line(0, n * cell_width, WIDTH, n * cell_width, fill=line_color)
def _cellsetup(self, nstart):
""" Spawns 'nstart' new cells and draws them """
for _ in range(nstart):
self._spawnnew()
def _lose(self):
""" Function called when the user is not able to continue the game """
self.main_canvas.create_text(WIDTH / 2, WIDTH / 2, text="GAME OVER", font=("Helvetica", 25), tag="d72819d9")
def _spawnnew(self):
""" Creates a new cell and draws it in an empty space """
while True:
pos = ri(0, 15) # Pick a random idx
if self.table[pos]:
# If the position is already taken, restart the loop
continue
posconv = pos % 4, int(pos / 4) # Converts the new idx into (x, y)
self.table[pos] = cell.Cell(self.main_canvas, self.root, posconv, WIDTH / 4, n=2 ** ri(1, 3))
break
# Let the user be able to click again
self._canclick = True
def _right(self):
""" Moves all the cells to the right side """
for idx in list(range(len(self.table)))[::-1]: # Iterates the array backwards
if self.table[idx]: # Checks if there's a cell
c = self.table[idx] # Saves the cell because 'idx' will change later
while (idx + 1) % 4: # Keeps going till it reaches the left side
# 1° CASE: Two cells add up
if self.table[idx + 1] and self.table[idx + 1].n == self.table[idx].n:
self.table[idx + 1].double() # Doubles a cell
self._scorevar.set(int(self._scorevar.get()) + self.table[idx + 1].n) # Updates the score label
self.table[idx] = 0 # Deletes the other
idx += 1
break
# 2° CASE: The cell stops
elif self.table[idx + 1]:
break
# 3° CASE: The cell moves to the left
else:
self.table[idx + 1] = self.table[idx]
self.table[idx] = 0
idx += 1
# Updates the canvas object of the cell and his '.pos' attribute
newx, newy = idx % 4, int(idx / 4)
c.move(newx - c.pos[0], newy - c.pos[1])
c.pos = newx, newy
def _left(self):
""" Moves all the cells to the left side """
for idx in range(len(self.table)): # # Iterates the array from the beginning
if self.table[idx]:
c = self.table[idx]
while idx % 4:
if self.table[idx - 1] and self.table[idx].n == self.table[idx - 1].n:
self.table[idx - 1].double()
self._scorevar.set(int(self._scorevar.get()) + self.table[idx - 1].n)
self.table[idx] = 0
idx -= 1
break
elif self.table[idx - 1]:
break
else:
self.table[idx - 1] = self.table[idx]
self.table[idx] = 0
idx -= 1
newx, newy = idx % 4, int(idx / 4)
c.move(newx - c.pos[0], newy - c.pos[1])
c.pos = newx, newy
def _down(self):
""" Moves all the cells to the bottom """
for idx in list(range(len(self.table)))[::-1]:
if self.table[idx]:
c = self.table[idx]
while not 12 <= idx <= 15:
if self.table[idx + 4] and self.table[idx + 4].n == self.table[idx].n:
self.table[idx + 4].double()
self._scorevar.set(int(self._scorevar.get()) + self.table[idx + 4].n)
self.table[idx] = 0
idx += 4
break
elif self.table[idx + 4]:
break
else:
self.table[idx + 4] = self.table[idx]
self.table[idx] = 0
idx += 4
newx, newy = idx % 4, int(idx / 4)
c.move(newx - c.pos[0], newy - c.pos[1])
c.pos = newx, newy
def _up(self):
""" Moves all the cells to the top """
for idx in range(len(self.table)):
if self.table[idx]:
c = self.table[idx]
while not 0 <= idx <= 3:
if self.table[idx - 4] and self.table[idx - 4].n == self.table[idx].n:
self.table[idx - 4].double()
self._scorevar.set(int(self._scorevar.get()) + self.table[idx - 4].n)
self.table[idx] = 0
idx -= 4
break
elif self.table[idx - 4]:
break
else:
self.table[idx - 4] = self.table[idx]
self.table[idx] = 0
idx -= 4
newx, newy = idx % 4, int(idx / 4)
c.move(newx - c.pos[0], newy - c.pos[1])
c.pos = newx, newy
root = Tk()
app = App(root)
root.bind("<a>", lambda event: app.callback(0))
root.bind("<d>", lambda event: app.callback(1))
root.bind("<w>", lambda event: app.callback(3))
root.bind("<s>", lambda event: app.callback(2))
root.bind("<r>", lambda event: app.restart())
root.mainloop()
-
1\$\begingroup\$ Congrats, it's a pretty good version. I never tried to write any Python GUI, this motivates me to try it! \$\endgroup\$Eric Duminil– Eric Duminil2018年02月21日 15:21:46 +00:00Commented Feb 21, 2018 at 15:21
1 Answer 1
Playability
For a game of 2048, I would expect that at least all of the tiles up to 2048 would have distinct colors.
The animation for each move is extremely annoying and confusing:
- The sliding movement is usually too slow, but is sometimes fast.
- The result of two equal tiles colliding should result in a merged tile, but the merged tile appears instantaneously, before the sliding animation completes.
- The random new tile usually appears before the sliding is complete. This often results in tiles sliding "underneath" the new tile.
When a new tile appears, it has value 2, 4, or 8 with equal probability. In an authentic 2048 game, it's usually 2, and occasionally 4, but never 8.
The "Game over" condition should be detected as soon as the board is full and no move is possible. You shouldn't have to wait until the user tries to make the next move.
Implementation
The Cell
class is misnamed; its name seems to refer to one of the fixed positions of the board, but actually refers to one of the movable items. In English, Tile
would be a more appropriate name.
Using a UUID for each Cell
object seems like overkill. Generating the id
from a global counter (perhaps from an itertools.count()
) would suffice.
Mapping left, right, down, and up to the numbers 0, 1, 2, and 3, respectively, and having App.callback()
map those numbers back to the methods ._left()
, ._right()
, ._down()
, and ._up()
is a pointless use of magic numbers. You could have represented the four directions as strings, and performed a method lookup based on the name.
Those four methods for moving cells in each direction are extremely similar to each other. They should be refactored to call a common handler that accepts (Δx, Δy) as parameters.
-
\$\begingroup\$ First of all thank you for your review, I thought it would have been much worse! All of your points are correct, I just want to say one thing about the movement functions: I literally spent DAYS thinking about how to create a single movement function, but I couldn't manage to find a solution... I ended up thinking that the whole way in which the game is structured (one single array where tiles are saved) is not the proper one. \$\endgroup\$Matteo Secco– Matteo Secco2018年02月20日 15:20:57 +00:00Commented Feb 20, 2018 at 15:20
-
\$\begingroup\$ @MatteoSecco I think your code could be easier to write with table as
[[0] * 4 for _ in range(4)]
instead of a flat list. The symmetry would be more apparent between all your methods. \$\endgroup\$Eric Duminil– Eric Duminil2018年02月21日 16:16:38 +00:00Commented Feb 21, 2018 at 16:16