6
\$\begingroup\$

I have written a program that will determine the height of a Tetris board after a sequence of moves are made. These inputs are in the form of a comma-delimited list, and look like <piece><position>. List of pieces:

  • I - this is a 1x4 piece lying on its side
  • Q - this is a 2x2 square piece
  • T - this is a T-shaped piece
  • Z - this is a left-facing 2x2 offset
  • S - this is a right-facing 2x2 offset
  • L - this is a right-facing L
  • J - this is a left-facing L

Image (source) of the pieces. Pieces are always in the same orientation as below. Visual representation of the shapes

I've diagrammed them out below as well. Rotation is not in scope for this problem (e.g. a vertical I is out of scope).

I - xxxx
Q - xx
    xx
T - xxx
     x
Z - xx
     xx
S -  xx
    xx
L - x
    x
    xx
J -  x
     x
    xx

Positions are 0-indexed, and represent a location from the left side of the board (the board is 10-wide).

Example 1:

Input: I0,Q4

Output: 2

Board:

bbbbQQbbbb
IIIIQQbbbb

(b represents a blank space, and the blank lines above this are left out)

Example 2

Input: Q0,Q2,Q4,Q6,Q8

Output: 0

Board (intentionally left blank):

Explanation: Using normal Tetris rules, a row is removed whenever every block in a row is filled. This sequence would place 5 square cubes evenly spaced along the bottom, which then removes those two rows.

class Tetris:
    def __init__(self):
        self.board =[]
        self.pieces = {
            'I' : [[1,1,1,1]],

            'Q' : [[1,1],
                   [1,1]],

            'T': [[1,1,1],
                  [0,1,0]],

            'Z':[[1,1,0],
                 [0,1,1]],

            'S':[[0,1,1],
                 [1,1,0]],

            'L':[[1,0],
                 [1,0],
                 [1,1]],

            'J':[[0,1],
                 [0,1],
                 [1,1]]}

    def newRow(self):
        return [0 for _ in range(10)]

    def doesThePieceFit(self,row,pieceName,pos):
        #checks to see if a piece fits on the row at given position
        #check bottom to the top
        piece = self.pieces[pieceName]
        for i in range(len(piece)):
           pieceRow = piece[-1*(1+i)]
           if i+row == len(self.board): return True
           boardRow = self.board[i+row]
           for j in range(len(pieceRow)):
               if pieceRow[j] and boardRow[pos+j]: return False
        return True

    def removeFullRows(self,startRow,numRows):
        #removes full rows from the board
        #only checks rows between startRow and startRow+numRows
        fullRows = [i+startRow
                    for i in range(numRows)
                    if all(self.board[i+startRow])]
        for fullRow  in sorted(fullRows,reverse=True):
            del self.board[fullRow]

    def addPieceAt(self,row,pieceName,pos):
        #Adds piece at this row.
        piece = self.pieces[pieceName]
        for i in range(len(piece)):
           pieceRow = piece[-1*(1+i)]
           if i+row == len(self.board):
               self.board+=self.newRow(),
           boardRow = self.board[i+row]
           for j in range(len(pieceRow)):
               if pieceRow[j]:
                   boardRow[pos+j] = pieceRow[j]
        self.removeFullRows(row,len(piece))

    def addPiece(self,pieceName,pos):
        #1.find the first row where piece is blocked
        #2.Add the piece at the row above it
        blockedByRow = None
        for row in range(len(self.board)-1,-1,-1):
            if not self.doesThePieceFit(row,pieceName,pos):
                blockedByRow = row
                break

        targetRow = 0 if  blockedByRow == None else blockedByRow+1
        self.addPieceAt(targetRow,pieceName,pos)

    def addPieces(self,pieces):
        for piece in pieces.split(','):
            self.addPiece(piece[0],int(piece[1]))
        return len(self.board)
\$\endgroup\$
  • 1
    \$\begingroup\$ This is a very good question, well detailed, interesting and even with images! You will become a great contributor of this site :) \$\endgroup\$ – Caridorc Jul 22 '20 at 19:15
  • \$\begingroup\$ Your second example board is empty? \$\endgroup\$ – Reinderien Jul 22 '20 at 19:18
  • 2
    \$\begingroup\$ @Reinderien yes - because all rows have been cleared \$\endgroup\$ – Dannnno Jul 22 '20 at 22:01
  • 1
    \$\begingroup\$ @Caridorc thank you. The credit goes to @ Dannno :D \$\endgroup\$ – meowlicious Jul 23 '20 at 1:23
2
\$\begingroup\$

The first thing I did was use Black to reformat the code - yours is pretty good, but there are some minor style complaints I had (generally around the lack of whitespace in a few places). Additionally, PEP8 defines the naming conventions in python - generally, prefer_this notThis.

Lastly, all of your methods should have docstrings. I haven't added this b/c it isn't as pertinent to the code review, but it is good practice in general.

From there, I thought about your actual approach. At a high level you:

  • Create a new instance of the object
  • Pass it a string, parse the string, and process each token
  • Attempt to fit pieces
  • Clear full rows

None of that is inherently bad, but I think it can be tightened up a bit.

User Input

Right now you don't have any validation of the user inputs - we're being very trusting that the values that are provided will be usable. We probably want to do this validation

Additionally, I don't think that the Tetris class should be responsible for handling the comma-delimited string - it should just take a piece and a position, and something else should be responsible for taking the input and translating it into arguments. If you're feeling friendly, a @classmethod might be appropriate. Lastly, I think this class method should return the board, not the height, so I added a new height property to the class. I ended up with something like this:

pieces = {
    "I": ((True, True, True, True)),
    "Q": ((True, True), (True, True)),
    "T": ((True, True, True), (False, True, False)),
    "Z": ((True, True, False), (False, True, True)),
    "S": ((False, True, True), (True, True, False)),
    "L": ((True, False), (True, False), (True, True)),
    "J": ((False, True), (False, True), (True, True)),
}

@classmethod
def add_pieces(cls, user_input):
    board = Tetris()
    for piece in user_input.split(","):
        if len(piece) > 2:
            raise ValueError(f"Piece {piece} is malformed")
        piece_id = piece[0]
        drop_position = piece[1]
        if not Tetris.is_valid_piece(piece_id):
            raise ValueError(f"Piece {piece_id} is not a valid Tetris piece")
        if not Tetris.is_valid_drop_location(drop_position):
            raise IndexError(
                f"Drop location {drop_position} is not a valid board location"
            )
        board.add_piece(piece_id, drop_position)
    return board

@classmethod
def is_valid_piece(cls, piece_id):
    return piece_id in cls.pieces

@classmethod
def is_valid_drop_location(drop_position):
    try:
        int(drop_position)
    except ValueError:
        return False

    return drop_position >= 0 and drop_position < 10

@property
def height(self):
    return self.board.length

You'll also notice that I moved Tetris.pieces into a class attribute instead of an instance attribute - this is because it should be the same everywhere. I also changed 0/1 to True/False because it is a binary value (I think an enum is probably best to be explicit, e.g. boardState.FULL and boardState.EMPTY). Lastly, I changed from nested lists to nested tuples - this is because tuples are immutable, and you never need to change the shape definition.

OOP

I wonder if it is worthwhile making a separate class to represent the pieces, and then you can do something like TetrisPiece.fitsAtLocation(board, location). I haven't fully thought about what this would look like or if it is actually better, but it might be a nice way to encapsulate that functionality.

This would also be a convenient way to extend this to handle rotations as well, as you would just do TetrisPiece.rotate(Direction.LEFT) and handle it all under the hood.

If you want to extend this to a full game, then instead of just having a "drop position" you also need a relative location on the board, handling T-spins, etc. The more complicated this gets, the more I think a separate class is going to improve readability.

General nitpicks

  • doesThePieceFit seems really weird - I get how it works, but you should definitely introduce some constants to replace the magic method, and maybe consider if there is a better way to model the data.
    • In particular, perhaps we should store the block state for a different shape in reverse order (e.g. bottom-to-top instead of top-to-bottom)?
  • removeFullRows creates a list, then sorts it - I think you can probably come up with a different approach for this
  • addPieceAt has the same magic as doesThePieceFit - is there a way that we can either combine their functionality, or use a common helper method?
  • addPiece I think you can use for-else to handle this a bit more elegantly than using the ternary, but my mood on the for-else swings every time I use it
\$\endgroup\$
2
\$\begingroup\$

Your code is good but it is not intuitive to interface with grafically.

I can print the board but it comes out reversed and as zeros and ones and I got to do:

>>> t = Tetris()
>>> print(t.board)

But you can use the special method repr to make it print nicely automagically (whenever the user asks print(t))

In Python 3 you can just add this at the end of your class:

class Tetris:
    # other code

    def __repr__(self):
        return '\n'.join(reversed([''.join("■" if elem else '□' for elem in line) for line in t.board]))

And now you have an intuitive and graphically nice pretty print:

t = Tetris()
for piece, pos in ( ('L',1), ('Z', 2), ('S', 3), ('I',5)):
    t.addPiece(piece, pos)
    print(t)
    print("\n"*5)

Outputs:

□■□□□□□□□□
□■□□□□□□□□
□■■□□□□□□□







□■□□□□□□□□
□■■■□□□□□□
□■■■■□□□□□






□□□□■■□□□□
□■□■■□□□□□
□■■■□□□□□□
□■■■■□□□□□






□□□□□■■■■□
□□□□■■□□□□
□■□■■□□□□□
□■■■□□□□□□
□■■■■□□□□□

In Python 2 you might have to use ASCII characters but this allows for easy developing and testing and is necessary in case you want to turn this into a game.

(It looks way nicer in Python IDLE than in this site).

\$\endgroup\$
0
\$\begingroup\$
  1. Use Booleans instead of Integers: The code uses integers to check if a cell is occupied or not. Example: Replace I = [1,1,1,1] with I=[True,True,True,True]

  2. Mark internal functions with underscores: By python convention, any function that is not meant to be invoked from outside the class is usually marked with underscores. Example: Replace def addPiece(...) with def _addPiece_(...).

  3. Use meaningful variable names: Use meaningful names for variables (including iterator variables) Don't use arbitrary names like i or j. Looking at the variable names, it isn't clear whether doesThePieceFit validates columns at all

  4. Handling invalid input: You can return an error value (throw a python error or return integer value -1) for invalid inputs. (Such as I9 on a size 10 board)


Also, if you can change the input format, you can make some minor changes to make this code more useful. You can change the constructor to __init__(self,size) instead of fixing the size to 10. Also, you can change the input format from string "Q0,Q2" to list [["Q",0],["Q",2]]

\$\endgroup\$
  • \$\begingroup\$ You should not use the __dunder__ method to mark "internal" functions. If you want, you can use _one_under to mark internal, and __two_under if you need name mangling too (you almost never need name mangling) \$\endgroup\$ – Dannnno Jul 22 '20 at 18:46
  • \$\begingroup\$ @Dannnno fixed it \$\endgroup\$ – Abhay Aravinda Jul 22 '20 at 18:49

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.