10
\$\begingroup\$

I have a function:

def palindrome(n):
    assert(n < 10)
    res = [['0']]
    for x in range(1, n + 1):
        str_x = str(x)
        l = len(res)
        res.insert(0, [str_x] * l)
        res.append([str_x] * l)
        for line in res:
            line.insert(0, str_x)
            line.append(str_x)
    return '\n'.join(
            ''.join(row) for row in res
            )

which creates strings like:

>palindrome(1)
111
101
111
>palindrome(7)
777777777777777
766666666666667
765555555555567
765444444444567
765433333334567
765432222234567
765432111234567
765432101234567
765432111234567
765432222234567
765433333334567
765444444444567
765555555555567
766666666666667
777777777777777

Is there any way to improve the code?

\$\endgroup\$
6
  • 4
    \$\begingroup\$ The first step might be to document what this function wants to achieve? You have some outputs, but right now I have to read the entire code to know what it is doing, since I cannot derive it from documentation or function name \$\endgroup\$
    – kushj
    Commented Jul 19, 2018 at 14:39
  • \$\begingroup\$ You might want to think about what the output should be for n > 9. Or if you even want to allow it. \$\endgroup\$
    – Graipher
    Commented Jul 19, 2018 at 15:01
  • \$\begingroup\$ @Graipher. I believe there is no proper answer for that question. The 'palindromes' which the function for n > 9 generates are still valid palindromes, but there is no way to show them nice \$\endgroup\$ Commented Jul 19, 2018 at 15:13
  • \$\begingroup\$ Which is why you should come up with an answer for your code. Either allow it (as currently), but maybe discourage it in the docstring, disallow it with an explicit check, generate different first and last lines, ... \$\endgroup\$
    – Graipher
    Commented Jul 19, 2018 at 15:14
  • 1
    \$\begingroup\$ @Graipher added an assertion \$\endgroup\$ Commented Jul 19, 2018 at 15:15

5 Answers 5

6
\$\begingroup\$

I'd recommend you start over. I always start patterns that are mirrored in the x axis by figuring out how to do a quarter of the pattern. I also use a view, so that if you want to change the icons you can.

And so I start with:

def palindrome(n):
    view = ''.join(map(str, range(n + 1)))
    for size in reversed(range(n + 1)):
        half = view[size:]
        print(half)

palindrome(3)
3
23
123
0123

After this we know to use view[size] as the center, and we repeat it size amount of times.

def palindrome(n):
    view = ''.join(map(str, range(n + 1)))
    for size in reversed(range(n + 1)):
        half = view[size] * size + view[size:]
        print(half)

palindrome(3)
3333
2223
1123
0123

From this you will then just have to mirror the output:

def palindrome(n):
    view = ''.join(map(str, range(n + 1)))
    for sizes in (reversed(range(n + 1)), range(1, n + 1)):
        for size in sizes:
            half = view[size] * size + view[size:]
            yield half[:0:-1] + half

Going forward, you could enter view into it and make it output whatever you want.

import itertools

def palindrome(view):
    n = len(view)
    for size in itertools.chain(reversed(range(n)), range(1, n)):
        half = view[size] * size + view[size:]
        yield half[:0:-1] + half

print('\n'.join(palindrome('.|.|')))
|||||||
|.....|
|.|||.|
|.|.|.|
|.|||.|
|.....|
|||||||
\$\endgroup\$
4
\$\begingroup\$

My answer will be similar to Mathias Ettinger, but I just want to explicitate that this is an example of maximum/Chebyshev distance, and this can simplify your explanation of the code.

Your numbers can be calculated by how far you are to the center, but the distance is measured as the maximum between the horizontal and vertical distances. So the code could be:

def dist(P1, P2):
    x1, y1 = P1
    x2, y2 = P2
    return max(abs(x2-x1), abs(y2-y1))

def palindrome(n, center=(0,0)):
    assert n <= 9

    return '\n'.join(
        ''.join(
            str(dist((x, y), center))
            for x in range(-n, n+1)
        )
        for y in range(-n, n+1)
    )

This reproduces your code, but if you change the center you can do nice things such as palindrome(3, center=(-2,-2))

1112345
1012345
1112345
2222345
3333345
4444445
5555555

This code is more general than it needs to, for the sake of explaining the reasoning. You can simplify by saying P2 is always (0,0) and removing the center argument.

\$\endgroup\$
2
\$\begingroup\$

You can simplify list manipulations by figuring out that you generate rows and numbers by decrementing from n then incrementing back to n; except that rows are limited to not decrement below a threshold. And that threshold follows the exact same pattern. So a first version could be:

def palindrome(n):
    def inner(upper):
        for x in range(n, 0, -1):
            yield str(max(x, upper))
        for x in range(n + 1):
            yield str(max(x, upper))
    return '\n'.join(
        ''.join(inner(x)) for x in range(n, 0, -1)
    ) + '\n'.join(
        ''.join(inner(x)) for x in range(n + 1)
    )

But there is too much repetitions, let's compute the full range up-front:

def palindrome(n):
    full_range = list(range(n, 0, -1)) + list(range(n + 1))
    def inner(upper):
        for x in full_range:
            yield str(max(x, upper))
    return '\n'.join(''.join(inner(x)) for x in full_range)

Or even

def palindrome(n):
    full_range = list(range(n, 0, -1)) + list(range(n + 1))
    return '\n'.join(''.join(str(max(x, y)) for y in full_range) for x in full_range)

But the usage of a double for loop over the same iterable can easily be replaced by itertools.product which is more efficient. You can also use itertools.chain instead of converting ranges to lists; and itertools.starmap will let us apply max over the pairs returned by product. But we would only get a single iterable that we would need to group together to form each line; so we would need the help of the grouper recipe:

import itertools


def grouper(iterable, size):
    args = [iter(iterable)] * size
    return zip(*args)


def palindrome(n):
    full_range = itertools.chain(range(n, 0, -1), range(n + 1))
    pattern = itertools.starmap(max, itertools.product(full_range, repeat=2))
    return '\n'.join(''.join(map(str, group)) for group in grouper(pattern, 2 * n + 1))
\$\endgroup\$
2
\$\begingroup\$

So you have a lot of list manipulation, I believe you can reduce that to work with strings and use re.sub to fill in the gaps.

Let's start by analyzing the answer:

I see that the result is basically symmetrical vertically and horizontally. Since lines are printed left-to-right, top-to-bottom, we can easily split it into top and bottom halves which are mirrored. This allows us to break apart our approach in only doing half the work, and copying a reflection onto itself.

Once res is populated with the top half of the answer, we want to extend it with the reversed copy of it. A common shorthand for a reversed list by using extended slices.

res.extend(res[::-1])

But doing this would copy the middle line twice! Since [::-1] is the same as [n:-1:-1], removing the last row would result in [n-1:-1:-1], or simply [n-1::-1].

res.extend(res[n-1::-1])

Now the top half of the solution can be broken down as repeated numbers, gradually substituting in decreased values for each row. The key word here is substitution. Creating a list and appending both sides is a way to handle this, but requires multiple modifications to it as you presented:

for x in range(1, n + 1):
    str_x = str(x)
    l = len(res)
    res.insert(0, [str_x] * l)
    res.append([str_x] * l)

Your approach appends the number to the beginning and end of the string. If you were working with strings, you could essentially create it all in one declaration. We can determine that the number of times n appears in the first line is 2n (since it's left-right mirrored) + 1 to account for the 0 that would be in the center. The starting line is very simple:

res = [str(n) * (n * 2 + 1)]

As mentioned earlier, the subsequent lines build on the previous line by replacing all but the first and last n-values with n-1. I really enjoy utilizing re.sub, but you can use string concatenation as well.

Using re, we could build a pattern as /n+/ where n is the number to be replaced. Once we find it, we need to replace it with n-1 using the same formula described above. But since we need to leave an n on each side, we'll have to subtract 2 from this formula to become 2 * n + 1 - 2 or simply 2 * n - 1.

n_minus_1_str = str(x-1) * (x * 2 - 1)

With this, we can replace all the middle n values with n_minus_1_str and attach n on both sides to preserve it.

str_x + n_minus_1_str + str_x

Putting this new line together for re.sub, we get:

new_line = re.sub(str_x + "+", str_x + (str(x-1) * (x * 2 - 1)) + str_x, res[-1])

You could also implement the above without re.sub, and use regular string concatenation instead to build the string. The slicing gets a bit tricky, but can be accomplished with:

new_line = res[-1][:n-x+1] + str(x-1) * (x * 2 - 1) + res[-1][x-n-1:]

Finally, we just need a for loop to run backwards from n down to 1 (not including 0, since we are adding n-1 in the loop.

for x in xrange(n, 0, -1):

Using xrange, since the list generated by range is unnecessary.

Putting this all together with '\n'.join(res) we get the following:

from re import sub
def palindrome(n):
    res = [str(n) * (n * 2 + 1)] # First line
    for x in xrange(n, 0, -1): # Append remaining until 0 is in the center
        str_x = str(x)
        res.append(sub(str_x + "+", str_x + (str(x-1) * (x * 2 - 1)) + str_x, res[-1]))
    res.extend(res[n-1::-1]) # Add the flipped version without the last row
    return '\n'.join(res)
print palindrome(8)

I chose to replace the entire string of x instead of matching only the middle values to save on regex computation.

Edit: Forgive me, first time posting in Code Review.

\$\endgroup\$
0
2
\$\begingroup\$
  • As others have said the your function palindrome does not explain what is does by its name nor by the non existing comments, this can partly be overcome by splitting the function in multiple functions with more descriptive names.

  • Instead of the append and insert methods of lists it might be clearer to use list concatenations with the + operator.

  • Sometimes a loop for creating lists can be more clearer with a list comprehension.

Keeping those in mind, my rewrite of your code:

# a palindrome in this context is a list of lists of chars which is
# highly symetrical, for instance :
# [['b', 'b', 'b'], ['b', 'a', 'b'], ['b', 'b', 'b']]

def expand_palindrome(palindrome, ch):
    n = len(palindrome)
    top_and_bottom = [[ch] * (n + 2)]
    middle_part = [[ch] + row + [ch] for row in palindrome]
    return top_and_bottom + middle_part + top_and_bottom

def create_number_palindrome(n):
    assert(n < 10)
    assert(n > 0)
    palindrome = [['0']]
    for i in range(n):
        palindrome = expand_palindrome(palindrome, str(i+1))
    return palindrome

def palindrome_string(palindrome):
    return '\n'.join(''.join(row) for row in palindrome)

print(palindrome_string(create_number_palindrome(1)))
print(palindrome_string(create_number_palindrome(7)))
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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