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()
\$\endgroup\$
  • 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 Feb 21 '18 at 15:21
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.

\$\endgroup\$
  • \$\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 Feb 20 '18 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 Feb 21 '18 at 16:16

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.