Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I am developing a 2048 program in Python. As a part of that project, I am working on merging method that merges the given integer entries.

I am interested in some suggestion to improve my code.

def creating_resulting_list(length_line):
    """
    Function that create a resulting_list with equal number of \
    zeros as length of line
    """
    from itertools import repeat
    return list(repeat(0, length_line))

#BEGIN Test
#print creating_resulting_list(2) == [0, 0]
#print creating_resulting_list(0) == []
#END Test

def moving_non_zeros(line, resulting_list):
    """
    Function that moves non zero entries in line \
    to left most in resulting_list
    """
    if not all(line):
        index_resulting_list = 0
        for dummy_value in line:
            if dummy_value:
                resulting_list[index_resulting_list] = dummy_value
                index_resulting_list += 1
    else:
        resulting_list = line[:]
    return resulting_list 

#BEGIN Test
#result_list = moving_non_zeros([4, 0, 4, 4], creating_resulting_list(4)) 

#result_list = moving_non_zeros([16, 16, 4, 4], creating_resulting_list(4)) 
#END Test

def locating_same_values(result_list):
    """
    Function that locates and merges the same values
    """
    for dummy_index, dummy_value in enumerate(result_list):
        if dummy_index+1 < len(result_list) and dummy_value == result_list[dummy_index+1]:
            result_list[dummy_index] += result_list[dummy_index+1]
            result_list[dummy_index+1] = 0
            if not len(result_list[dummy_index+1:]) > 2:
                return moving_non_zeros(result_list, creating_resulting_list(len(result_list)))
    return result_list

#BEGIN Test

print locating_same_values([16, 16, 4, 4])
print locating_same_values([2, 2, 2, 2, 2])
#END Test

def merge(line):
    """
    Function that merges a single row or column in 2048.
    """
    result_list = creating_resulting_list(len(line))
    result_list = moving_non_zeros(line, result_list)
    return locating_same_values(result_list)
share|improve this question
up vote 8 down vote accepted

I'm just going to review creating_resulting_list.

def creating_resulting_list(length_line):
    """
    Function that create a resulting_list with equal number of \
    zeros as length of line
    """
    from itertools import repeat
    return list(repeat(0, length_line))

#BEGIN Test
#print creating_resulting_list(2) == [0, 0]
#print creating_resulting_list(0) == []
#END Test
  1. There's no need to use backslashes at the ends of lines inside a triple-quoted string.

  2. The name creating_resulting_list doesn't help the reader understand what it does. What is a "resulting list" anyway? What this function does is to create a list of zeros. So a better name would be zeros_list or just zeros (see for example numpy.zeros, which is similar).

  3. The argument is called length_line. But this is too specific: nothing in this function cares about lines. By avoiding dependencies like this, you can make code more self-contained and easier to understand in isolation. So I would call this argument just length.

  4. Since the import statement is inside this function, it gets run every time the function is called. If you moved this statement to the top level of your module, then it would be run only once.

  5. The test cases should be made automatically runnable, using the unittest or doctest modules. I'll show below how to do it using the latter.

  6. The sequence documentation says:

    s * n or n * s    equivalent to adding s to itself n times

    so you don't need itertools.repeat, you can just multiply.

Revised code with doctests:

def zeros(length):
    """Create and return a list of zeros with the given length.

    >>> zeros(2)
    [0, 0]
    >>> zeros(0)
    []

    """
    return [0] * length

The doctests can be run using python -m doctest program.py.

share|improve this answer
    
Thanks for your well-explained response. Can you please tell me what is doctest? – Karthikeyan Palaniswamy Jan 10 at 18:06
    
@KarthikeyanPalaniswamy: See the doctest documentation. – Gareth Rees Jan 10 at 19:08

filter

A lot of the time Python lets you forget about list indexes and appending, in favour of things like list comprehensions and higher-level functions: saying what you want instead of how.

Your moving_non_zeros can be collapsed into this:

def moving_non_zeros(line, zeros):
    result = list(filter(bool, line)) + zeros    # Do the actual moving
    changes = len(line) - len(result)            # Append the correct number of 0s
    return result + (zeros[:changes] if changes else [])

because result = list(filter(bool, line)) is the same as this:

result = []
index = 0
for dummy_value in line:
    if bool(dummy_value):    # which is `if dummy_value:`
        result[index] = dummy_value
        dummy_value += 1

Or, using chain from itertools,

def moving_non_zeros(line, zeros):
    result = chain(filter(bool, line),
                   zeros)
    return list(islice(result, len(line)))

repeat

This:

from itertools import repeat
return list(repeat(0, length_line))

can be simplified to the following, since you're just returning a list instead of a generator:

return [0] * length_line

Naming

In general, I recommend just using index and value instead of dummy_index and dummy_value -- it's shorter, and usually implied that these loop variables are "dummy" variables anyway.

Otherwise, your code formatting and layout looks perfectly fine to me.

Zero-moving algorithm; .pop()

I'm not sure that moving_non_zeros is really necessary. Right now you're using 0 as a flag in locating_same_values, where, for example, [2, 2, 8] becomes [4, 0, 8] which you then have to rearrange so the zeroes float to the right.

I think things would be simpler if you use pop() to just remove the second number in a match, and then append however many zeroes you need to once you've finished. This way, unless I've missed a detail of 2048, your entire program becomes a slightly modified version of locating_same_values:

def merge(line):
    """
    Merges a single row or column in 2048.
    """
    nums = line[:]
    i = 0
    while i + 1 < len(nums):
        if nums[i] == nums[i + 1]:
            nums[i] += nums.pop(i + 1)
        i += 1
    # 0-pad from the right to same length as original line
    return nums + [0] * (len(line) - len(nums))

which could still be split into maybe two smaller functions if you prefer.

share|improve this answer
    
Thank you so much – Karthikeyan Palaniswamy Jan 10 at 18:38
    
your moving_non_zeros gives wrong answer. – Karthikeyan Palaniswamy Jan 10 at 18:52
    
@KarthikeyanPalaniswamy Sorry! I have corrected it so it returns the same length list as the input. – BenC Jan 11 at 5:31

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.