4
\$\begingroup\$

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:

  1. Use of list comprehensions
  2. Use of Python's built-in truthiness
  3. Minimal branches (fewer if(s), else(s), etc.)
  4. Infusing new and useful methods like extend into my code
  5. Improving the code style by enhancing readability
  6. 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.

\$\endgroup\$
  • \$\begingroup\$ You could deal with pylint errors by making pylint configuration file. On of configuration is describing good names. \$\endgroup\$ – kushj Sep 13 '15 at 9:22
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ A description of the doctest doesn’t belong in the function docstring, -> it does. Having “no output” mean success could be a problem -> no, it is good common practice, run tests in verbose mode if it bothers you. \$\endgroup\$ – Caridorc Sep 14 '15 at 10:45

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.