Plz comment on this code

Alex Willmer alex at moreati.org.uk
Sun Sep 19 12:54:18 CEST 2010


Your code works (assuming digits gets populated fully), but it's the
absolute bare minimum that would.
To be brutally honest it's:
 - unpythonic - you've not used the core features of Python at all,
such as for loops over a sequence
 - poorly formatted - Please read the python style guide and follow it
 - not reusable - Your code can only be called from the command line,
it should be usable as a module
 - not documented - There is no indication what this code does, other
than mentally running it
 - Fragile - There is no error checking on user input

There are other ways to write what you have more concisely (e.g. list
comprehensions, iterators) but those can wait for another time. Here
is a start at improving your code wrt to the above points:

#!/usr/bin/env python3

# bigdigits2.py

ZERO = ["***", # NB Constants are by convention ALL_CAPS
        "* *",
        "***"]
ONE = ["** ",
       " * ",
       "***"]

# TODO Define and populate digits 2-9
DIGITS = [ZERO, ONE, ZERO, ONE, ZERO, ONE, ZERO, ONE, ZERO, ONE]

def big_digits(str_of_digits):
    """Return a list of lines representing the digits using 3x3 blocks
of "*"
    """
    banner = [] # Accumulate results in this

    # Loop over the rows/lines of the result
    # TODO Replace hard coded block size with global constant or
measured size
    for row in range(3):
        line_parts = []

        # Assemble the current line from the current row of each big
digit
        for digit in str_of_digits:
            big_digit = DIGITS[int(digit)]
            line_parts.append(big_digit[row])

        # Create a string for the current row and add it to the result
        line = " ".join(line_parts)
        banner.append(line)

    return banner

def usage():
    print("Usage: bigdigit.py <number>", file=sys.stderr)
    sys.exit(1)

if __name__ == "__main__":
    import sys

    # Check that an argument was passed
    # NB This will ignore additional arguments
    if len(sys.argv) >= 2:
        input_string = sys.argv[1]
    else:
        usage()

    # Check that only digits were passed
    if not input_string.isnumeric():
        usage()

    # All is well, print the output
    for line in big_digits(input_string):
        print(line)

Here are some suggested further improvements:
- Map directly from a digit to it's big digit with a dictionary,
rather than indexing into a list:
BIG_DIGITS = {
    "1": ["** ",
          " * ",
          "***"],
    # ...
    }
- Is input_string.isnumeric() the right test? Can you find a character
it would not correctly flag as invalid input?
- What if I wanted to use my own 4x4 big digits? Could the
big_digits() function accept it as an argument?



More information about the Python-list mailing list