# RattleHiss (fizzbuzz in python)

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

I've now been programming in python for all of 4 hours.

At 70 lines it's rather verbose for a FizzBuzz. If you can suggest a more pythonic way to structure it, that would be awesome.

## FizzBuzz.py

def print_fizz_buzz_output(start_end_pair, pair_list):

start_num, end_num = start_end_pair
for num in range(start_num, end_num + 1):
has_divisor = False
for divisor, text in pair_list:
if num % divisor == 0:
has_divisor = True
print(text, end='')
if has_divisor:
print() # New Line
else:
print(str(num))

exit_flag = False
while not exit_flag:
divisor_is_valid = False
while not divisor_is_valid:
divisor = input('Divisor? ')
try:
divisor = int(divisor)
divisor_is_valid = True
except ValueError:
print('Invalid input for divisor. Divisor must be a whole number. Please try again.')

text = input('Text? ')
yield (divisor, text)

while True:
continue_response = input('Input Another Divisor (y/n)? ')
if continue_response in ('N', 'n'):
exit_flag = True
break
elif continue_response in ('Y', 'y'):
exit_flag = False
break
else:
print('Invalid response. Please input "y" or "n"')

start_num_is_valid = False
while not start_num_is_valid:
start_num = input('Start Number? ')
try:
start_num = int(start_num)
start_num_is_valid = True
except ValueError:
print('Invalid input for start number. Must be a whole number. Please try again.')

end_num_is_valid = False
while not end_num_is_valid:
end_num = input('End Number? ')
try:
end_num = int(end_num)
if end_num >= start_num:
end_num_is_valid = True
else:
raise ValueError
except ValueError:
print('Invalid input for end number. Must be a whole number greater than or equal to the start number.'

return start_num, end_num



## Example Input/Output

Start Number? 1
End Number? 20
Divisor? 3
Text? Rattle
Input Another Divisor (y/n)? y
Divisor? 5
Text? Hiss
Input Another Divisor (y/n)? n
1
2
Rattle
4
Hiss
Rattle
7
8
Rattle
Hiss
11
Rattle
13
14
RattleHiss
16
17
Rattle
19
Hiss

• As you are new to python, you should be informed that the name refers not to snakes, but to the Monty Python group. As such, instead of foo, bar, and baz, we use spam, eggs, and bacon as generic variable names. I'm not sure what would replace fizz and buzz. – Casey Kuball Oct 5 '16 at 18:01

# Flags, flags everywhere

There are constructs in Python that can reduce the amount of needed flags like the for ... else blocks or the try .. else construct. break and return should also be your friends:

def print_fizz_buzz_output(start_end_pair, pair_list):

start_num, end_num = start_end_pair
for num in range(start_num, end_num + 1):
has_divisor = False
for divisor, text in pair_list:
if num % divisor == 0:
has_divisor = True
print(text, end='')
if has_divisor:
print() # New Line
else:
print(str(num))

while True:
while True:
divisor = input('Divisor? ')
try:
divisor = int(divisor)
except ValueError:
print('Invalid input for divisor. Divisor must be a whole number. Please try again.')
else:
break

text = input('Text? ')
yield (divisor, text)

while True:
continue_response = input('Input Another Divisor (y/n)? ')
if continue_response in ('N', 'n'):
return
elif continue_response in ('Y', 'y'):
break
else:
print('Invalid response. Please input "y" or "n"')

while True:
start_num = input('Start Number? ')
try:
start_num = int(start_num)
except ValueError:
print('Invalid input for start number. Must be a whole number. Please try again.')
else:
break

while True:
end_num = input('End Number? ')
try:
end_num = int(end_num)
except ValueError:
print('Invalid input for end number. Must be a whole number.'
else:
if end_num < start_num:
print('Invalid input for end number. Must be greater than or equal to the start number. Please try again.')
else:
break

return start_num, end_num



# Factorize out common behaviour

Next we see that you make extensive use of constructs like:

while True:
value = input('Something')
try:
result = convert(value)
except ValueError:
print('Error message')
else:
break


You can factorize that out:

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

def print_fizz_buzz_output(start_end_pair, pair_list):
start_num, end_num = start_end_pair
for num in range(start_num, end_num + 1):
has_divisor = False
for divisor, text in pair_list:
if num % divisor == 0:
has_divisor = True
print(text, end='')
if has_divisor:
print() # New Line
else:
print(str(num))

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

while True:
continue_response = input('Input Another Divisor (y/n)? ')
if continue_response in ('N', 'n'):
return
elif continue_response in ('Y', 'y'):
break
else:
print('Invalid response. Please input "y" or "n"')

start_num = ask_something('Start Number? ', error_message='Invalid input for start number. Must be a whole number. Please try again.')

while True:
end_num = ask_something('End Number? ', error_message='Invalid input for end number. Must be a whole number. Please try again.')
if end_num < start_num:
print('Invalid input for end number. Must be greater than or equal to the start number. Please try again.')
else:
break

return start_num, end_num



# Build strings before printing them

The last flag we didn't take care of is in the print_fizz_buzz_output function. For this one, we need to see the inner for loop as something equivalent to:

text_for_divisor = []
for divisor, text in pair_list:
if num % divisor == 0:
text_for_divisor.append(text)
if text_for_divisor:
print(''.join(text_for_divisor))
else:
print(num)


So we got rid of the flag but the construct is inneficient. Better use a list-comprehension instead:

text_for_divisor = [text for divisor, text in pair_list if num % divisor == 0]
print(''.join(text_for_divisor) if text_for_divisor else num)


# Use if __name__ == '__main__'

@Dex'ter already told you, but trully, you want to take that habit. When you jump into an interactive session and try to debug your functions, you don't want to be prompted about what are your bounds. You want to be able to directly do:

>>> import FizzBuzz as fb
>>> fb.print_fizz_buzz_output((1, 50), [(3, 'Fizz'), (5, 'Buzz')])


# Better signature

I would use print_fizz_buzz_output(begin, end, pair_list) instead of requiring a tuple as the begin/end pair, this feel more natural. You would then need to change your main call to print_fizz_buzz_output(*ask_iteration_range(), list(ask_divisor_text_pairs()))

# Proposed improvements

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

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

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

while True:
continue_response = input('Input Another Divisor (y/n)? ')
if continue_response in ('N', 'n'):
return
elif continue_response in ('Y', 'y'):
break
else:
print('Invalid response. Please input "y" or "n"')

start_num = ask_something('Start Number? ', error_message='Invalid input for start number. Must be a whole number. Please try again.')

while True:
end_num = ask_something('End Number? ', error_message='Invalid input for end number. Must be a whole number. Please try again.')
if end_num < start_num:
print('Invalid input for end number. Must be greater than or equal to the start number. Please try again.')
else:
break

return start_num, end_num

if __name__ == '__main__':

• Personally, I find for ... else to be a very hard construct to read. It may be quite expressive, but I always have to check whether the else runs on broken or normal exit. As such I'm personally more likely to check the variable for whatever I was looking for rather than rely on for ... else. – CAD97 Oct 5 '16 at 21:36
• @CAD97 The else feels neater than "initializing + setting + testing a flag". But I understand than presenting it as for .. else or even while .. else is confusing; it should really be break .. else. Same with try .. else which should be presented as except .. else. – 301_Moved_Permanently Oct 5 '16 at 22:51
• Note: Your rewrite of the signature requires PEP-0448 which is only available with Python 3.5+. – Jonas Schäfer Oct 6 '16 at 10:56
• @MathiasEttinger That is a great way to present … else! I should memorise that when people again complain about the oddness of that construct. – Jonas Schäfer Oct 6 '16 at 10:57
• @JonasWielicki True, but you can support earlier versions by unpacking before the function call. The line being very long, it won't hurt: begin, end = ask_iteration_range(); print_fizz_buzz_output(begin, end, ...) – 301_Moved_Permanently Oct 6 '16 at 16:32

You should try to DRY your code. You have three occurrences of duplicate code. To get the start, end and divisor you near copy and pasted the code across all three. Instead I'd recommend that you use a function.

The function should have the question you ask in input, and the message when you enter incorrect data. You can also pass it a function to convert the input to. And pass exception(s) for it to re-try on.

def get_input(question='', error_msg='', conversion=int, exception=ValueError):
while True:
try:
return conversion(input(question))
except exception:
if error_msg:
print(error_msg)


I'd also remove exit_flag from ask_divisor_text_pairs, as return will exit from both loops and the function fine. This allows you to remove a variable and a few lines of code. Which makes the function look quite small now with the addition of the above function.

I'd also recommend that you learn how to perform comprehensions. They're like for loops, but build data in a more readable, and normally more performant, way. The iterative way to do the equivalent could be:

my_list = []
for i in range(10):
if i % 2 == 0:
my_list.append(i)


Which as a list comprehension would become:

my_list = [i for i in range(10) if i % 2 == 0]


This allows us to change your divisor loop. You can change the code so that for each number between the entered domain it builds a list of divisors, rather than print them. You can then go on to check if the list is empty or not, and print either the divisors or the current number. Increasing the readability of your function. (I was a little confused about the empty print.) This allows you to get:

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


I only made a couple of changes but here is all the changes I made:

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

def get_input(question='', error_msg='', conversion=int, exception=ValueError):
while True:
try:
return conversion(input(question))
except exception:
if error_msg:
print(error_msg)

while True:
divisor = get_input('Divisor? ', 'Invalid input for divisor. Divisor must be a whole number. Please try again.')
text = input('Text? ')
yield (divisor, text)
while True:
continue_response = input('Input Another Divisor (y/n)? ')
if continue_response in ('N', 'n'):
return
elif continue_response in ('Y', 'y'):
break
else:
print('Invalid response. Please input "y" or "n"')

start = get_input('Start Number? ', 'Invalid input for start number. Must be a whole number. Please try again.')
while True:
end = get_input('End Number? ', 'Invalid input for end number. Must be a whole number. Please try again.')
if end >= start:
break
print('Invalid input for end number. Must be greater than or equal to the start number. Please try again.')

return start, end



print() in python takes all types/objects/things in python as argument, as far as I know..

I perhaps would have one input function. That toke the Terminal title, and type for try catch( if you send int or str non called, you can call it later in the function with x() ), and the error message. If possible ofc.

If you use annotations use them all the time.

You probably know this but intuitively but

the lines following and including.

if continue_response in ('N', 'n'):


could be

if continue_response.lower() in ('n'):


I see your point with the rows

if end_num >= start_num:
end_num_is_valid = True
else:
raise ValueError


in the try clause but i don't like it.

But overall I like it a lot. It fine.

• Dammit! ^^ That was indeed a silly mistake. – Simon Oct 5 '16 at 16:36
• This fizzbuzz problem remind me much of the first problem of project euler, difference being that you add the numbers. I do it in a couple of different ways on my ugly page, however it's django!.. If I remember correctly sets would be the fastest method in this problem. simon.otur.nu/code/euler/1 – Simon Oct 5 '16 at 16:38