5
\$\begingroup\$

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
\$\endgroup\$

5 Answers 5

Reset to default
3
\$\begingroup\$

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()))))
\$\endgroup\$
4
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ I just wanted to say that I wouldn't necessarily advise you to import multiple functions on the same line. Some style guides (including Google's) frown upon that. \$\endgroup\$ Commented Oct 6, 2016 at 19:32
  • \$\begingroup\$ @yoniLavi Google's says to explicitly not do import os, sys, just like PEP8. PEP8 says importing multiple identifiers in one import is fine, Google's don't say explicitly say either way. \$\endgroup\$ Commented Oct 6, 2016 at 19:44
3
\$\begingroup\$

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.

\$\endgroup\$
3
\$\begingroup\$

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.

\$\endgroup\$
1
\$\begingroup\$

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.

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.