5
\$\begingroup\$

I have made a replica of the game 2048. It works exactly how the game should work. However I was just looking to know where I could've possibly cleaned up my code or make it more efficient.

import random
board =([0,0,0,0],
        [0,0,0,0],
        [0,0,0,0],
        [0,0,0,0])



def pieces_test(board):
    places = find_non_zeros(board)
    print(places)


def combine_numbers(board, value, new_y, new_x, del_y, del_x):
    value *= 2
    board[new_y][new_x] = value
    board[del_y][del_x] = 0
    return board

def find_zeros(board):
    good_places = []
    for i in range(len(board)):
        for j in range(len(board[i])):
            if board[i][j] == 0:
                good_places.append([i,j])
    return good_places


def find_non_zeros(board):
    good_places = []
    for i in range(len(board)):
        for j in range(len(board[i])):
            if board[i][j] != 0:
                good_places.append([i,j])
    return good_places



def start_of_game(board):
    for i in range(2):
        places = find_zeros(board)
        new_coord = random.choice(places)
        board[new_coord[0]][new_coord[1]] = 2
    return board

def move_left(board):
    not_left = True
    unusable_pieces = []
    while not_left:
        not_left = False
        pieces_positions = find_non_zeros(board)
        #pieces_positions = [[piece1_y, piece1_x],[piece2_y, piece2_x]...]
        for i in range(len(pieces_positions)):
            if pieces_positions[i][1] != 0:
                    if board[pieces_positions[i][0]][pieces_positions[i][1]-1] == 0:
                        if [pieces_positions[i][0],pieces_positions[i][1]] in unusable_pieces:
                            index_value = unusable_pieces.index([pieces_positions[i][0],pieces_positions[i][1]])
                            unusable_pieces[index_value][1] -= 1
                        value = board[pieces_positions[i][0]][pieces_positions[i][1]]
                        board[pieces_positions[i][0]][pieces_positions[i][1]-1] = value
                        board[pieces_positions[i][0]][pieces_positions[i][1]] = 0
                        not_left = True
                    elif board[pieces_positions[i][0]][pieces_positions[i][1]-1] == board[pieces_positions[i][0]][pieces_positions[i][1]]:
                        if [pieces_positions[i][0],pieces_positions[i][1]] not in unusable_pieces:
                            if [pieces_positions[i][0], pieces_positions[i][1]-1] not in unusable_pieces:
                                value = board[pieces_positions[i][0]][pieces_positions[i][1]]
                                combine_numbers(board, value, pieces_positions[i][0], pieces_positions[i][1]-1, pieces_positions[i][0], pieces_positions[i][1])
                                unusable_pieces.append([pieces_positions[i][0], pieces_positions[i][1]-1])
                                not_left = True




                #       combine_numbers(board, value_of_board_pieces, new_y, new_x, del_y, del_x)



    return board

def move_right(board):
    not_right = True
    unusable_pieces = []
    while not_right:
        not_right = False
        pieces_positions = find_non_zeros(board)
        pieces_positions = pieces_positions[::-1]
        #pieces_positions = [[piece1_y, piece1_x],[piece2_y, piece2_x]...]
        for i in range(len(pieces_positions)):
            if pieces_positions[i][1] != 3:
                    if board[pieces_positions[i][0]][pieces_positions[i][1]+1] == 0:
                        if [pieces_positions[i][0],pieces_positions[i][1]] in unusable_pieces:
                            index_value = unusable_pieces.index([pieces_positions[i][0],pieces_positions[i][1]])
                            unusable_pieces[index_value][1] += 1
                        value = board[pieces_positions[i][0]][pieces_positions[i][1]]
                        board[pieces_positions[i][0]][pieces_positions[i][1]+1] = value
                        board[pieces_positions[i][0]][pieces_positions[i][1]] = 0
                        not_right = True
                    elif board[pieces_positions[i][0]][pieces_positions[i][1]+1] == board[pieces_positions[i][0]][pieces_positions[i][1]]:
                        if [pieces_positions[i][0],pieces_positions[i][1]] not in unusable_pieces:
                            if [pieces_positions[i][0], pieces_positions[i][1]+1] not in unusable_pieces:
                                value = board[pieces_positions[i][0]][pieces_positions[i][1]]
                                combine_numbers(board, value, pieces_positions[i][0], pieces_positions[i][1]+1, pieces_positions[i][0], pieces_positions[i][1])
                                unusable_pieces.append([pieces_positions[i][0], pieces_positions[i][1]+1])
                                not_right = True

    return board

def move_up(board):
    not_up = True
    unusable_pieces = []
    while not_up:
        not_up = False
        pieces_positions = find_non_zeros(board)
        #pieces_positions = [[piece1_y, piece1_x],[piece2_y, piece2_x]...]
        for i in range(len(pieces_positions)):
            if pieces_positions[i][0] != 0:
                if board[pieces_positions[i][0]-1][pieces_positions[i][1]] == 0:
                    if [pieces_positions[i][0],pieces_positions[i][1]] in unusable_pieces:
                        index_value = unusable_pieces.index([pieces_positions[i][0],pieces_positions[i][1]])
                        unusable_pieces[index_value][0] -= 1
                    value = board[pieces_positions[i][0]][pieces_positions[i][1]]
                    board[pieces_positions[i][0]-1][pieces_positions[i][1]] = value
                    board[pieces_positions[i][0]][pieces_positions[i][1]] = 0
                    not_up = True
                elif board[pieces_positions[i][0]-1][pieces_positions[i][1]] == board[pieces_positions[i][0]][pieces_positions[i][1]]:
                    if [pieces_positions[i][0],pieces_positions[i][1]] not in unusable_pieces:
                        if [pieces_positions[i][0]-1, pieces_positions[i][1]] not in unusable_pieces:
                            value = board[pieces_positions[i][0]][pieces_positions[i][1]]
                            combine_numbers(board, value, pieces_positions[i][0]-1, pieces_positions[i][1], pieces_positions[i][0], pieces_positions[i][1])
                            unusable_pieces.append([pieces_positions[i][0]-1, pieces_positions[i][1]])
                            not_up = True

    return board

def move_down(board):
    not_down = True
    unusable_pieces = []
    while not_down:
        not_down = False
        pieces_positions = find_non_zeros(board)
        pieces_positions = pieces_positions[::-1]
        #pieces_positions = [[piece1_y, piece1_x],[piece2_y, piece2_x]...]
        for i in range(len(pieces_positions)):
            if pieces_positions[i][0] != 3:
                    if board[pieces_positions[i][0]+1][pieces_positions[i][1]] == 0:
                        if [pieces_positions[i][0],pieces_positions[i][1]] in unusable_pieces:
                            index_value = unusable_pieces.index([pieces_positions[i][0],pieces_positions[i][1]])
                            unusable_pieces[index_value][0] += 1
                        value = board[pieces_positions[i][0]][pieces_positions[i][1]]
                        board[pieces_positions[i][0]+1][pieces_positions[i][1]] = value
                        board[pieces_positions[i][0]][pieces_positions[i][1]] = 0
                        not_down = True
                    elif board[pieces_positions[i][0]+1][pieces_positions[i][1]] == board[pieces_positions[i][0]][pieces_positions[i][1]]:
                        if [pieces_positions[i][0],pieces_positions[i][1]] not in unusable_pieces:
                            if [pieces_positions[i][0]+1, pieces_positions[i][1]] not in unusable_pieces:
                                value = board[pieces_positions[i][0]][pieces_positions[i][1]]
                                combine_numbers(board, value, pieces_positions[i][0]+1, pieces_positions[i][1], pieces_positions[i][0], pieces_positions[i][1])
                                unusable_pieces.append([pieces_positions[i][0]+1, pieces_positions[i][1]])
                                not_down = True

    return board

def add_piece(board):
    free_spots = find_zeros(board)
    chosen_spot = random.choice(free_spots)
    board[chosen_spot[0]][chosen_spot[1]] = 2
    return board            





def main(board):
    board = start_of_game(board)
    game_finished= False
    while game_finished == False:
        if find_zeros(board) == []:
            game_finished = True
            continue
        print(board[0],"\n",board[1],"\n",board[2],"\n",board[3])
        move = input("do you want to move up, down, left or right")
        if move == "up":
            move_up(board)
        if move == "down":
            move_down(board)
        if move == "left":
            move_left(board)
        if move == "right":
            move_right(board)
        board = add_piece(board)


main(board)
\$\endgroup\$
4
\$\begingroup\$

The thing that jumps out when reading this is the use of indexers: a snippet like this:

 if board[pieces_positions[i][0]][pieces_positions[i][1]-1] == 0:
    if [pieces_positions[i][0],pieces_positions[i][1]] in unusable_pieces:
        index_value = unusable_pieces.index([pieces_positions[i][0],pieces_positions[i][1]])
        unusable_pieces[index_value][1] -= 1

is extremely hard to read (and hence to debug) because of the onslaught of brackets (as an informal rule of thumb, if the code scrolls off to the right in a StackOverflow code window, that's a sign you should see how to tighten it up).

You could eliminate a lot of that by using python's built-in unpacking syntax. Since pieces_positions is a list-of-lists, you can tame it by looping over it like this:

 for y, x in pieces_positions: 
     #.....

Now hard to read stuff like

  board[pieces_positions[i][0]][pieces_positions[i][1]-1]

becomes

  board[y][x-1]

with that change your four main functions look more readable:

def move_right(board):
    not_right = True
    unusable_pieces = []
    while not_right:
        not_right = False
        pieces_positions = find_non_zeros(board)
        pieces_positions = pieces_positions[::-1]
        #pieces_positions = [[piece1_y, piece1_x],[piece2_y, piece2_x]...]
        for y, x in pieces_positions:
            if x != 3:
                if board[y][x+1] == 0:
                    if [y,x] in unusable_pieces:
                        index_value = unusable_pieces.index([y,x])
                        unusable_pieces[index_value][1] += 1
                    value = board[y][x]
                    board[y][x+1] = value
                    board[y][x] = 0
                    not_right = True
                elif board[y][x+1] == board[y][x]:
                    if [y,x] not in unusable_pieces:
                        if [y, x+1] not in unusable_pieces:
                            value = board[y][x]
                            combine_numbers(board, value, y, x+1, y, x)
                            unusable_pieces.append([y, x+1])
                            not_right = True

    return board

However, the four main functions are all basically symmetrical -- they only differ in the direction of movement and the limit condition. It'd be cleaner and easier to debug if you could get them down to a single function that was used everywhere.

In this case, you can express the movement more clearly by treating it as a pair of offsets added to your x and y values -- say, a single move(board, offset_x, offset_y) function instead of the four you currently have. Then you can generalize the boundaries in something like this fashion:

for y, x in pieces_positions:

    next_y = y + offset_y
    next_x = x + offset_x

    # python has a nice chained comparison that 
    # is great for checking a range
    valid_x = -1 < next_x < 4
    valid_y = -1 < next_y < 4

    if not (valid_x and valid_y):
        continue

This has the added advantage of making the actual changes to the board clearer too:

if board[next_y][next_x] == 0:
    if [y,x] in unusable_pieces:
        index_value = unusable_pieces.index([y,x])
        if offset_y = 0:   # this means an x-move
            unusable_pieces[index_value][1] += offset_x
        else:   # a y move
            unusable_pieces[index_value][0] += offset_y

    value = board[y][x]
    board[next_y][next_x] = value
    board[y][x] = 0

Working through the unpacked values instead of range() and a single general function instead of four functions that vary only on a couple of number placements will make the game a lot easier to read and thus to debug and maintain.

\$\endgroup\$
0
\$\begingroup\$

as a late review, i congratulate you for your separation of tasks, apart from the point made by @theodox, just if something is not going to be immutable, you can wrap it in a list

from

   ([0,0,0,0],
    [0,0,0,0],
    [0,0,0,0],
    [0,0,0,0])

to

   [[0,0,0,0],
    [0,0,0,0],
    [0,0,0,0],
    [0,0,0,0]]

as it sends off the wrong message to code readers

\$\endgroup\$

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.