Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have the following function defined in a program that implements the 2048 game. I tried condensing the repeating part of the code into one loop containing +1 offset variables for the indices, which are either set to 0 or 1 depending on the direction passed into the function, but the outer and inner loop variables swap depending on the direction. Could anyone give me some pointers as to how to condense this code and make it more clean, pythonic and terse?

def merge(direction):
    #if adjacent cells are equal, they are merged into one with twice the value and then moved.
    if direction == "up":
        for col in range(nColumns):
            for row in range(nRows-1):
                if grid[row][col] == grid[row+1][col]:
                    grid[row][col] = grid[row+1][col]*2
                    grid[row+1][col] = 0
                    move(direction)

    if direction == "down":
        for col in range(nColumns):
            for row in range(nRows-1):
                if grid[row][col] == grid[row+1][col]:
                    grid[row+1][col] = grid[row][col]*2
                    grid[row][col] = 0
                    move(direction)

    if direction == "left":
        for row in range(nRows):
            for col in range(nColumns-1):
                if grid[row][col] == grid[row][col+1]:
                    grid[row][col] = grid[row][col]*2
                    grid[row][col+1] = 0
                    move(direction)

    if direction == "right":
        for row in range(nRows):
            for col in range(nColumns-1):
                if grid[row][col] == grid[row][col+1]:
                    grid[row][col+1] = grid[row][col]*2
                    grid[row][col] = 0
                    move(direction)
share|improve this question
    
Shouldn't your if be more indented? –  Morwenn Mar 28 at 14:30
    
@Morwenn The lack of indentation evidently unintentional, so I've fixed it. –  200_success Mar 28 at 15:00
1  
In one implementation I had seen, there was a trick like turning the board in one direction and then in the other in order to deal with moves in only one direction. For instance, if you have implemented "move_down", to do "move_right", you just need to : rotate clockwise, move_down and rotate anti-clockwise –  Josay Mar 28 at 15:35

2 Answers 2

up vote 5 down vote accepted

Iterating more smartly can simplify your loops.

To start with, run this standalone example and see that the izip(…) expression produces useful pairs of coordinates:

from itertools import product, izip
nRows, nColumns = 3, 5
for dest, src in izip(product(range(nRows - 1), range(nColumns)),
                      product(range(1, nRows), range(nColumns))):
    print(dest, src)

Then, you can reuse the loop just by changing which ranges you pass in.

from itertools import product, izip

def merge(direction):
    def merge(dest_iterator, src_iterator):
        for (dr, dc), (sr, sc) in izip(dest_iterator, src_iterator):
            if grid[dr][dc] == grid[sr][sc]:
                 grid[dr][dc] == 2 * grid[sr][sc]
                 grid[sr][sc] = 0
                 move(direction)

    if direction == "up":
        merge(product(range(nRows - 1), range(nColumns)),
              product(range(1, nRows), range(nColumns)))
    elif direction == "down":
        merge(product(range(1, nRows), range(nColumns)),
              product(range(nRows - 1), range(nColumns)))
    elif direction == "left":
        merge(product(range(nRows), range(nColumns - 1)),
              product(range(nRows), range(1, nColumns)))
    elif direction == "down":
        merge(product(range(nRows), range(1, nColumns)),
              product(range(nRows), range(nColumns - 1)))
share|improve this answer
    
Thank you for acquainting me with itertools. I can't say the solution is completely intuitive yet, but that will come in time. –  ohBoy Mar 28 at 19:38

You could use a dictionary of lambdas to find locations of accepting and donating cells in the grid.. Then place the operations in try/except, that will simply fail if you go out of bounds

GetLocations = dict(
    up = lambda r,c: (r,c,r+1,c ),
    down = lambda r,c: (r+1,c,r,c ),
    right = lambda r,c: (r,c,r,c+1),
    left = lambda r,c: (r,c+1,r,c ))

def merge(direction):
    for col in range(nColumns):
        for row in range(nRows):
            #get acceptor and doner locations
            ar,ac,dr,dc = GetLocations[direction](row,col)
            try:
                #if the evaluation doesn't fail, nothing will fail
                if grid[ar][ac] == grid[dr][dc]:
                    grid[ar][ac] = grid[dr][dc]*2
                    grid[dr][dc] = 0
                    move(direction)
            except IndexError:
                pass
share|improve this answer
1  
I suggest catching just IndexError to avoid swallowing other exceptions. –  200_success Mar 28 at 15:43
1  
I suggest don't making it throw IndexError in the first place by checking the bounds before you break them. –  Simon André Forsberg Mar 28 at 15:44
    
Good call,.. Added it! –  Calpratt Mar 28 at 15:45
1  
Thank you! This looks like what my approach would have been. I think you got right and left switched up. –  ohBoy Mar 28 at 19:40

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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