6
\$\begingroup\$

First Code Review request =) I wrote this after making several projects that required a user to pick an option. I wanted to have a set way of forcing that choice in all of my projects. The module also contains a GetNumberChoice() func, but I'll save that for a separate post.

As a relatively green programmer, I'm looking for feedback on the Pythonic-ness, the structure of my functions, and any tips to improve my coding going forward. I really appreciate it!

#!/usr/bin/env python

"""
This is part of a module that contains tools for getting input from a user. At any point while getting input, the user may enter "quit", "exit", or "leave" to quit()
"""
_EXIT_WORDS = ["quit", "exit", "leave"]


def GetStringChoice(prompt, **kwoptions):
    """
    Print out the prompt and then return the input as long as it matches one of the options (given as key/value pairs)

    Example call:
        >>> prompt = "Who is the strongest Avenger?"
        >>> input_options = {"t":"Thor", "i":"Iron Man", "c":"Captain America", "h":"The Hulk"}
        >>> response = GetStringChoice(prompt, **input_options)
        Who is the strongest Avenger?
        - 't' for 'Thor'
        - 'i' for 'Iron Man'
        - 'c' for 'Captain America'
        - 'h' for 'The Hulk'
        h
        >>> response
        'h'

    Invalid results are rejected:
        >>> response = GetStringChoice(prompt, **input_options)
        Who is the strongest Avenger?
        - 't' for 'Thor'
        - 'i' for 'Iron Man'
        - 'c' for 'Captain America'
        - 'h' for 'The Hulk'
        Ant-Man
        That wasn't one of the options.
        Who is the strongest Avenger?
        ...
    """
    OPTION_TEMPLATE = " - '{0}' for '{1}'" #used to display the kwoptions
    MAX_LINE_LEN = 60
    PAD = len(OPTION_TEMPLATE) - 6 
    # "- 6" in PAD removes the characters from the formatting brackets. I'm sure 
    #   there's a better way of doing this... Previously I just hardcoded this to
    #   be 11 (the number of characters always in OPTION_TEMPLATE) but that seemed
    #   worse/less understandable

    #This adjusts the section before the hyphen to be as wide as the longest key.
    space = max(map(len, kwoptions))

    while True:
        try:
            print(_TruncateAtMax(prompt))
            for key in kwoptions:
                #The _TruncateAtMax call adjusts the section after the hypen to be no longer than 60
                #  characters, and indents any overflow lines so that they start at the same place
                #  as the parent line starts.
                print(OPTION_TEMPLATE.format(key.ljust(space), \
                    _TruncateAtMax(kwoptions[key], max_len=MAX_LINE_LEN, spacer=space + PAD)))

            user_choice = input("")
            if user_choice in kwoptions:
                return user_choice
            elif user_choice in _EXIT_WORDS:
                quit()

            print("That wasn't one of the options.",)
        except TypeError:
            print("\n\n")
            raise
        except SystemExit:
            print("Thanks!")
            raise SystemExit #raise the SystemExit exception again to exit the program

def _TruncateAtMax(str, max_len=80, spacer=1):
    """
    Truncate a string at max length and continue on the next line. For example:
    >>>string = "this is a test of the truncate func"
    >>>max_len = 15
    >>>_TruncateAtMax(string, max_len)
    > this is a test 
    > of the truncate
    > func
    """
    if len(str) <= max_len - spacer:
        return str

    display_str = []
    next_line = ""
    spacing = "\n" + (" " * spacer)
    terms = str.split()

    for term in terms:
        if len(next_line + term) < max_len:
            next_line += term + " "
        else:
            display_str.append(next_line)
            next_line = term + " "
    else:
        #adds any stragglers (if next_line != "" but also !< max at the end of terms)
        display_str.append(next_line) 

    truncated_str = spacing.join(display_str)

    return truncated_str[1:] if truncated_str[0] == "\n" else truncated_str[:]
\$\endgroup\$
  • 1
    \$\begingroup\$ Backslash is redundant inside parentheses. According to PEP-8 it backslashes should be avoided when possible. Also, passing empty string to input() is redundant. \$\endgroup\$ – belkka Feb 17 '19 at 13:29
4
\$\begingroup\$

The code looks clean and is fairly readable. Good job!

Now here are some things you might want to consider:

  1. You use _EXIT_WORDS only to check membership. Instead of a list, use a set:

    _EXIT_WORDS = { 'leave', 'stop', 'quit' }
    

    Set membership is O(1) whereas list membership is O(n), so it's just a good habit to be in.

  2. You spent some time writing the function _TruncateAtMax but python ships with the textwrap module that does pretty much what you want. The best code is code you don't have to write yourself...

  3. Instead of a long explanatory comment after this line:

    PAD = len(OPTION_TEMPLATE) - 6 
    

    why not just make it explicit:

    PAD = len(OPTION_TEMPLATE) - len('{0}{1}')
    

    or use @belkka's excellent suggestion:

    PAD = len(OPTION_TEMPLATE.format('', ''))
    

    And instead of calling it PAD, which is nicely short, see if you can name it something a bit more clear.

  4. Instead of calling key.ljust(space) for each key, try just using the space as a parameter to the format string. You can do something like: '{0:{1}}'.format(str, width)

  5. Instead of re-computing the strings each time, why not compute the strings outside the loop and print them inside as needed?

  6. Instead of raising SystemExit why not just raise again? You do it for TypeError...

\$\endgroup\$
  • 2
    \$\begingroup\$ Another way to calculate PAD: len(OPTION_TEMPLATE.format('', '')) \$\endgroup\$ – belkka Feb 17 '19 at 13:23
  • \$\begingroup\$ @AustinHastings, first off, thanks for the feedback! Your suggestions make a lot of sense. For #4, what's the benefit of using .format() over .ljust? Could you explain #5 a little more? What strings should I be computing outside the loop? \$\endgroup\$ – DukeSilver Feb 19 '19 at 0:06
  • 1
    \$\begingroup\$ @DukeSilver the advantage of format is that you're already using it. So instead of calling format, but first calling ljust to pad one of the parameters, you can say, "format, do this stuff, and make sure the field for this one string is this-many characters wide" as part of a single operation. The strings to compute outside the loop are the strings you are computing with format and ljust and everything. Your loop is infinite, but you're just printing the same data over and over again. So compute the data only once. \$\endgroup\$ – aghast Feb 19 '19 at 4:14

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.