patternpythonMinor
2048 Pythonic Merge Function
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:
Sidenote: Pylint yells at me for using
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
extendinto 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:
-
Likewise
-
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
-
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.
-
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.