Tetris logic for interview

Question

I would appreciate feedback on the code below which I created during an one-hour live interview session. In particular, while the interviewer pretty much said nothing during the interview, their feedback was that the code was overcomplicated at times. Any other feedback/improvements is also greatly appreciated.

Some background info: currently two years of experience in a non-tech company doing software development, mostly Python. The problem of the interview is to implement some Tetris functionality, such as rotate right/left and clear lines (as a follow up); he said no event loops for game play for simplicity. We also didn't run the code. I have put what I said verbally in the interview in the comments.

### these define the type of blocks I could receive, the value defines the relative position of brick relative to the centre of mass. uncompleted 
BRICK_TYPES = {
 'a': (),
 'b': ((-2, 0), (-1, 0), (0, 0), (0, 1)),
 'c': (),
}
### the brick state would comprised of its vertical and horizontal position, as well as where its brick is relative to centre of mass, so it starts with the predefined state. 
class Brick:
 def __init__(self, b_type: str):
 self.type = b_type
 self.x = 0
 self.y = 0
 if b_type not in BRICK_TYPES:
 raise KeyError("Brick type not valid")
 self.elements = BRICK_TYPES[b_type]
 self.prev_state = None
 self.curr_state = (self.x, self.y, self.elements)
 def update_state(self):
 self.curr_state = (self.x, self.y, self.elements)
 def reverse_state(self):
 self.curr_state = self.prev_state
 self.prev_state = None
 self.x, self.y, self.elements = self.curr_state
 @staticmethod
 def get_element_positions(state):
 x, y, elements = state
 return tuple((x + element[0], y + element[1]) for element in elements)
 def move_left(self):
 self.x -= 1
 self.prev_state = self.curr_state
 def move_right(self):
 self.x += 1
 self.prev_state = self.curr_state
### the rotation is done by multiplying the rotation matrix like in vector rotation, also uncompleted since I cannot remember the constant
 def rotate(self, clockwise: bool):
 clockwise_rotate_matrix = [[1, -1], [-1, 1]]
 anticlockwise_rotate_matrix = [[1, -1], [-1, 1]]
 self.elements = tuple([element @ (clockwise_rotate_matrix if clockwise else anticlockwise_rotate_matrix)
 for element in self.elements])
 self.prev_state = self.curr_state
### the board will take height/width and keep track of current brick, as well as a state that store whether a brick occupies the space or not.
class Board:
 def __init__(self, height: int, width: int):
 self.height = height
 self.width = width
 self.bricks = []
 self.curr_brick = None
 self.board_state = [[0] * self.width for _ in range(self.height)]
### skipped since he said it's not necessary
 def run(self):
 pass
 def control(self, key_stroke: str):
 pass
### remove previous position and update it to the new position
 def update_board_state(self):
 curr_brick = self.curr_brick
 prev_positions = curr_brick.get_element_positions(curr_brick.prev_state) if curr_brick.prev_state is not None else ()
 new_positions = curr_brick.get_element_positions(curr_brick.curr_state)
 
 for prev_position in prev_positions:
 self.board_state[prev_position[1]][prev_position[0]] = 0
 for new_position in new_positions:
 self.board_state[new_position[1]][new_position[0]] = 1
### decide which rows to clear
 def cleared_rows(self):
 curr_positions = self.curr_brick.get_element_positions(self.curr_brick.curr_state)
 relevant_y_coords = {curr_position[1] for curr_position in curr_positions}
 cleared_rows = []
 for y_coord in relevant_y_coords:
 if all(self.board_state[y_coord]):
 cleared_rows.append(y_coord)
 return cleared_rows
### clear rows by counting the index to see how many rows it will fall then map it to its new position (e.g. clearing row 2, row 5 means 3->2, 4->3, 6->4, 7->5 etc.) , and if it's not replaced by another row, then clear the rows entirely
 def clear_rows(self):
 cleared_rows = self.cleared_rows()
 remap_rows = {}
 
 for row in cleared_rows:
 for r in range(row, self.height):
 remap_rows[r] = remap_rows.get(r, r) - 1
 
 for original_row in sorted(remap_rows.keys()):
 self.board_state[remap_rows[original_row]] = self.board_state[original_row]
 
 old_rows = remap_rows.keys()
 new_rows = remap_rows.values()
 for row in set(old_rows).difference(set(new_rows)):
 self.board_state[row] = [0] * self.width
### if collide, reverse to previous state; otherwise updates the board and perform row clearing
 def move(self, move_type: str):
 if move_type == 'left':
 self.curr_brick.move_left()
 elif move_type == 'right':
 self.curr_brick.move_right()
 elif move_type == 'rotate clockwise':
 self.curr_brick.rotate(True)
 elif move_type == 'rotate anticlockwise':
 self.curr_brick.rotate(False)
 else:
 raise KeyError(f"Move {move_type} not supported")
 if self.check_collision():
 self.curr_brick.reverse_state()
 else:
 self.curr_brick.update_state()
 self.update_board_state()
 self.clear_rows()
 def in_range(self, x: int, y: int):
 return 0 <= x < self.width and 0 <= y < self.height
### check if the move will result in overlapping with existing bricks
 def check_collision(self):
 positions = self.curr_brick.get_element_positions(self.curr_brick.curr_state)
 return any(not self.in_range(*position) or self.board_state[position[1]][position[0]] for position in positions)

• ggorlen: What is the specification/purpose of this? In other words, can you elaborate on what exact "tetris logic" is to be implemented here? Are there tests, or how are these classes used?
• charlielao: it's an interview question implementing some aspects of the Tetris game, including rotate, move left/right and clear rows; I meant to get some feedback on my interview performance so the codes are not really complete with test cases and runnable examples etc. Apologies for the confusion.
• STerliakov: Since it's a quick interview question, I'd completely get rid of rotation implementation and just hardcode four positions per element. It's difficult to get rotation correct without tests and running code, as you might have noticed, and there are only few pieces in tetris - no harm from just hardcoding them.

Answer 1

Very quickly,

• KISS, I would have checked if the brick can move before changing state, this way I could delete all the reverse_state logic

• KISS, is rotate anti clock wise a thing?

• Good, check_collision is quite concise, I like it

• KISS, You will have keyboard keys (int or string), which you map to strings like "rotate anticlockwise", which you map to function calls, I think you can drop 1 level there

• curr_positions = self.curr_brick.get_element_positions(self.curr_brick.curr_state) KISS, why is get_element_positions a function of curr_brick, all info comes from the state.

  Now imagine a world where you see positions = get_positions(self.brick)

• Semantics, this is hard to read, why use 'brick', we are not dropping bricks but pieces, initially I thought a piece was made of bricks and could not make head or tails

• What is BRICK_TYPES['a'] about?

• Mast: Your second point: clockwise_rotate_matrix if clockwise else anticlockwise_rotate_matrix apparently so.
• charlielao: thanks for the feedback! particularly 1st and 5th points are very helpful. and I agree anticlockwise is probably not a thing in the real game; the last one is about selecting predefined block topology and yes I agree I wasn't very clear on the naming of the variables.

Answer 2

I trust your interviewer displayed good manners and complimented you on your hard work under pressure. I hope you got an offer.

Documentation

Now that you have time to breathe, you can convert those comments into docstrings, and split them into multiple lines. For example:

"""
These define the type of blocks I could receive.
The value defines the position of bricks relative to the centre of mass.
""" 

A docstring summarizing the purpose of each class would also be helpful.

Naming

You did a terrific job in naming your classes, functions and variables given the time constraints you were under. Here are some small suggestions for improvement.

BRICK_TYPES is descriptive, but a, b and c could use some further explanation.

b_type would be better as brick_type.

x and y are fine, but I would add comments explicitly stating that they are horizontal and vertical coordinates on the board. Also, state the significance of their default 0 values, for example, bottom left on the board (if that is the case).

cleared_rows is a good name for the variable, but it might be better to rename the function of the same name as something like:

def get_cleared_rows(self):

curr_brick can probably be simplified to brick since I don't see any use of a variable named prev_brick or the like.

When I read this line, I think that you will perform a rotation:

 self.curr_brick.rotate(True)

but when I read this line, I don't think you will do a rotation:

 self.curr_brick.rotate(False)

It is great that you used a bool for the function input, but maybe using an enum with names like CLOCK_WISE and COUNTER_CLOCK_WISE would be more descriptive.

Input validation

It is great that you check for proper input:

 if b_type not in BRICK_TYPES:
 raise KeyError("Brick type not valid")

It would be helpful to the user to also echo the erroneous input value in the error message:

 raise KeyError(f"Brick type {b_type} not valid")

Perhaps you could also display the valid values in the message.

• charlielao: thanks very much for the feedback regarding code readability, the interviewer unfortunately barely said anything and just said it's too complicated after rejection