HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythonMinor

2048 Pythonic Merge Function

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
pythonicfunction2048merge

Problem

This post goes over how I updated my 2048 merge function code from spaghetti code to being somewhat more readable.

Spaghetti Old Code

I incorporated a few techniques from pretty much all of your suggestions! I realized there were a lot of logical mistakes and redundancy in my code, like why append 0's over and over again when it's not necessary at all, because in the end I back-track, account for missing numbers, and then append 0(s) in their place.

Improvements in new code vs. old code:

  • Use of list comprehensions



  • Use of Python's built-in truthiness



  • Minimal branches (fewer if(s), else(s), etc.)



  • Infusing new and useful methods like extend into my code



  • Improving the code style by enhancing readability



  • Removing redundandcy and flawed logic



def merge(nums):
    '''
    Takes a list as input
    returns merged pairs with
    non zero values shifted to the left.
    fancy interactive doc test below, no output means no problems.
    >>> merge([2, 0, 2, 4])
    [4, 4, 0, 0]
    >>> merge([0, 0, 2, 2])
    [4, 0, 0, 0]
    >>> merge([2, 2, 0, 0])
    [4, 0, 0, 0]
    >>> merge([2, 2, 2, 2, 2])
    [4, 4, 2, 0, 0]
    >>> merge([8, 16, 16, 8])
    [8, 32, 8, 0]
    '''
    slide = [num for num in nums if num]
    pairs = []
    for idx, num in enumerate(slide):
        if idx == len(slide)-1:
            pairs.append(num)
            break
        elif num == slide[idx+1]:
            pairs.append(num*2)
            slide[idx+1] = None
        else:
            pairs.append(num)  # Even if not pair you must append
    slide = [pair for pair in pairs if pair] 
    slide.extend([0] * (len(nums) - len(slide)))
    return slide

if __name__ == '__main__':
    import doctest
    doctest.testmod()


Sidenote: Pylint yells at me for using i instead of idx.

Solution

I didn’t review or see your old code when it was posted, but this is noticeably improved. However, I can still make a few suggestions:

-
num and nums are not great variable names. Characters are cheap; just write out the word number in full. I would also suggest giving numbers a slightly different/longer name to make it easier to read the line where you define slide.

Likewise pair/pairs could be a little easier to read, I think.

-
A few comments on the different branches to explain why you’re behaving in a particular way would be useful – to somebody unfamiliar with the rules of 2048, it might not be immediately obvious why it behaves the way it does.

-
A description of the doctest doesn’t belong in the function docstring, it probably belongs in a comment on the main() function. If I import your function into another file and then inspect merge.__doc__, the mention of a doctest “below” could be confusing.

-
Having “no output” mean success could be a problem, because there’s no way for me to distinguish between a success and the test not running. It should be totally clear whether your test passed, failed, or didn’t run. Anything else can hide problems.

Context

StackExchange Code Review Q#104551, answer score: 2

Revisions (0)

No revisions yet.