4
\$\begingroup\$

I am learning Python online on Stepik platform, and unfortunately I don't have any opportunity to get reviews from the tutor. The platform only checks the correctness of the answer that my program gives. So I decided to share the code here. Comments and corrections would be much appreciated.

Write a program that print multiplication table fragment for all numbers in the interval [a; b] for all of the interval [c; d]

def print_mult_table(a, b, c, d):
    """
    Print multiplication table fragment for all numbers in the interval
    [:param a; :param b] for all of the interval [:param c; :param d]
    """
    m = list(list([i] + list(range(c * i, (d + 1) * i, i))) for i in ([1] + list(range(a, b + 1))))
    m[0][0] = ""    # This is done, because it is the desired output format
    max_width = len(str(m[-1][-1])) + 1
    for i in m:
        i = [str(j).rjust(max_width) for j in i]
        print('\t'.join(i))
\$\endgroup\$
2
\$\begingroup\$

I would make also this function a generator and make printing the responsibility of the caller. This makes the function more reusable.

def print_mult_table(row_start, row_end, col_start, col_end):
    """
    Print multiplication table fragment for all numbers in the interval
    [:param row_start; :param row_end] for all of the interval [:param col_start; :param col_end]
    """
    table = list(list([row] + list(range(col_start * row, (col_end + 1) * row, row))) for row in ([1] + list(range(row_start, row_end + 1))))
    table[0][0] = ""    # This is done, because it is the desired output format
    max_width = len(str(table[-1][-1])) + 1
    for row in table:
        yield '\t'.join(str(col).rjust(max_width) for col in row)

for row in print_mult_table(1, 10, 4, 8):
    print(row)

Note that I gave the variables easier to understand names. This makes the list comprehension longer. Which leads to the next point:

While the list comprehensions are nice and compact (with short incomprehensible variable names), they are starting to stretch what is still easily readable. A general rule is, if your list comprehension is longer than a line (80 characters) it is probably too complicated. Therefore I would unfold it:

from itertools import chain


def get_mult_table(row_start, row_end, col_start, col_end):
    """
    Yield multiplication table fragment for all numbers in the interval
    [:param row_start; :param row_end] for all of the interval
    [:param col_start; :param col_end]
    """
    for row in chain([1], range(row_start, row_end + 1)):
        yield [col * row for col in chain([1], range(col_start, col_end + 1))]


def print_mult_table(row_start, row_end, col_start, col_end):
    table = list(get_mult_table(row_start, row_end, col_start, col_end))
    table[0][0] = ""    # This is done, because it is the desired output format
    max_width = len(str(table[-1][-1])) + 1
    for row in table:
        print '\t'.join(str(col).rjust(max_width) for col in row)

if __name__ == '__main__':
    print_mult_table(1, 10, 4, 8)

This makes it a lot more obvious what happens (because it includes the actual multiplication, it is not hidden in the step size of the inner range). This might make it slightly slower (when generating multiplication tables from, say, 1 to 1,000,000).

It also separates the generation of the table from the actual printing logic.

\$\endgroup\$

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.