11
\$\begingroup\$

enter image description here

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()
asked Feb 19, 2018 at 18:44
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Congrats, it's a pretty good version. I never tried to write any Python GUI, this motivates me to try it! \$\endgroup\$ Commented Feb 21, 2018 at 15:21

1 Answer 1

10
\$\begingroup\$

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.

answered Feb 20, 2018 at 7:16
\$\endgroup\$
2
  • \$\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\$ Commented 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\$ Commented Feb 21, 2018 at 16:16

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.