MontyPython (Fizzbuzz)

Question

This is an iterative review. The previous iteration can be found here.

---

I've now been programming in python for all of 6 hours.
This is probably my last iteration of FizzBuzz.

Thoughts?

---

FizzBuzz.py

from utilities import ask_something
from utilities import ask_yes_no


def print_fizz_buzz_output(start_num, end_num, pair_list):
    for num in range(start_num, end_num + 1):
        divisor_text = [text for divisor, text in pair_list if num % divisor == 0]
        if divisor_text:
            print(''.join(divisor_text))
        else:
            print(num)


def ask_divisor_text_pairs():
    loop = True
    while loop:
        divisor = ask_something('Divisor? ', int,
                                'Invalid input for divisor. Divisor must be a whole number. Please try again.')
        text = ask_something('Text? ', str)
        yield (divisor, text)

        loop = (True if 'y' == ask_yes_no('Input another Divisor (y/n)? ') else False)


def ask_iteration_range() -> tuple:
    start_num = ask_something('Start Number? ', int)
    while True:
        end_num = ask_something('End Number? ', int)
        if end_num >= start_num:
            break
        else:
            print('Invalid input for end number. Must be a whole number greater than or equal to the start number.'
                  ' Please try again.')
    return start_num, end_num


if __name__ == '__main__':
    print_fizz_buzz_output(*ask_iteration_range(), list(ask_divisor_text_pairs()))

---

utilities.py

def ask_something(prompt: str, convert, error_message='Invalid input.'):
    while True:
        value = input(prompt)
        try:
            return convert(value)
        except ValueError:
            print(error_message)


def ask_yes_no(prompt: str, allow_upper_case=True) -> str:
    # returns 'y' or 'n'
    while True:
        value = ask_something(prompt, str)
        if value == 'y' or (value == 'Y' and allow_upper_case):
            return 'y'
        elif value == 'n' or (value == 'N' and allow_upper_case):
            return 'n'
        else:
            print('Invalid Input. Please input "y" or "n"')

---

Example Input/Output

Start Number? 1
End Number? 15
Divisor? 3
Text? Monty
Input another Divisor (y/n)? y
Divisor? 5
Text? Python
Input another Divisor (y/n)? n
1
2
Monty
4
Python
Monty
7
8
Monty
Python
11
Monty
13
14
MontyPython

Answer 1

You could start making your code reusable by other modules.

As it stand, your "real" FizzBuzz code is contained in print_fizz_buzz_output, the rest being a mean to grab parameters values from the user.

I could use this code to provide my own wrapper such as:

import argparse
from FizzBuzz import print_fizz_buzz_output as fizzbuzz


def parse_command_line():
    parser = argparse.ArgumentParser(description='Customizable FizzBuzz')
    parser.add_argument('-b', '--begin', type=int, default=1, help='starting value')
    parser.add_argument('-e', '--end', type=int, default=20, help='stopping value')
    parser.add_argument('-d', '--divisor', nargs=2, action='append', help='couple of divisor/associated text')
    args = parser.parse_args()

    if args.end < args.begin:
        parser.error('end should be after begin')

    if not args.divisor:
        args.divisor = [(3, 'Fizz'), (5, 'Buzz')]  # Default value
    else:
        try:
            args.divisor = [(int(divisor), text) for divisor, text in args.divisor]
        except ValueError as e:
            parser.error(e)

    return args


if __name__ == '__main__':
    args = parse_command_line()
    fizzbuzz(args.begin, args.end, args.divisor)

However, print_fizz_buzz_output directly prints the result. So I cannot use it if I want to do anything else. You’d be better off either:

1. returning a list of all the values; or
2. turning that function into a generator and yielding results instead.

I’d choose the second option since it allows for better reusability and you can still call list on the generator if you trully want a list there:

def fizzbuzz(start_num, end_num, divisor_text_pairs):
    for num in range(start_num, end_num + 1):
        divisor_text = [
                text
                for divisor, text in divisor_text_pairs
                if num % divisor == 0
        ]
        if divisor_text:
            yield ''.join(divisor_text)
        else:
            yield num

and change your main to either:

if __name__ == '__main__':
    for element in fizzbuzz(*ask_iteration_range(), list(ask_divisor_text_pairs())):
        print(element)

or

if __name__ == '__main__':
    print('\n'.join(fizzbuzz(*ask_iteration_range(), list(ask_divisor_text_pairs()))))

Answer 2

The code is good!

---

I however do have a couple of niggles:

• You should use commas to import multiple functions from a module. This changes the imports at the top to a single import:

  from utilities import ask_something, ask_yes_no
  

• == results in a boolean, no need to make on yourself.

  >>> ('y' == 'y', 'y' == 'n')
  (True, False)
  

  And so you can simplify the reassignment of the loop flag.

• If I already can't read half of a string, you may as well keep it all on one line. Whilst PEP8 says that we should limit the line length to 79 characters, formatting large chunks of words within this requirement become an eye saw to me. And so I'd just leave them on one line.

  I also don't really like the verbose messages:

  Invalid input for divisor. Divisor must be a whole number. Please try again.

  The message "The input must be a whole number." is enough. Alternately you could just re-ask the question, which implies there's been an error.

Answer 3

One small thing:

ask_yes_no asks for a confirmation, and returns y or n.

You can make is easier to use by returning a boolean directly, so the caller does not need to make the comparison.

Then you can rename it to ask_confirm. This allows you to abstract the notion of asking a confirmation with how it is done. You can then consider changing the implementation, and for example support other languages, with localized responses which would not need to be y/n.

Answer 4

Overall, the code is quite good, but I have a few comments (in addition to the other good comments above):

• First of all, it doesn't run (at least on my Python 3.4), due to a syntax error:

  SyntaxError: only named arguments may follow *expression

  You should change it to something like this, which (imho) would also improve readability:

  start, end = ask_iteration_range()
  print_fizz_buzz_output(start, end, ask_divisor_text_pairs())
  

• (As I hinted in the previous line) I'd avoid the generator (yield) in ask_divisor_text_pairs. Generators are generally only used to avoid storing the items in memory, and using one here when not needed might confuse the readers, since it's turned into a list immediately. You can instead just directly collect them into a list:

  def ask_divisor_text_pairs():
      divisor_pairs = []
      while True:
          divisor = ask_something('Divisor? ', int,
                                  'Invalid input for divisor. Divisor must be a whole number. Please try again.')
          text = ask_something('Text? ', str)
          divisor_pairs.append((divisor, text))
  
          if not ask_yes_no('Input another Divisor (y/n)? '):
              return divisor_pairs
  

  Also, for slightly better UX, I'd probably prefer to collect each divisor pair on a single line (perhaps separated by a single space) and terminate the input after a sentinel value, such as an empty line, or an EOF. But that's obviously orthogonal to using Python.

• In ask_yes_no you have a comment on the first line. These kinds of comments, which are crucial to knowing how to use a function, should be docstrings, which syntactically just means that you should use a regular string instead of a comment.

• Also, not a Python issue - in favor of the YAGNI principle, I'd avoid the allow_upper_case parameter in ask_yes_no. But if you do want to keep it, then note that you should change the error message (Please input "y" or "n") to change based on that.

Answer 5

That's not bad.

I like that the input functions actually check the entered input matches the wanted type. Though in some cases a user might like to be able to abort some action at the prompt, so returning None or some default value for an empty input might be one choice.

The one thing where I succeeded in breaking it was by entering a divisor of zero.