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))


def ask_divisor_text_pairs():
    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"')


def ask_iteration_range() -> tuple:

    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.'
                  ' Please try again.')

    return start_num, end_num


print_fizz_buzz_output(ask_iteration_range(),list(ask_divisor_text_pairs()))

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

Answer

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))


def ask_divisor_text_pairs():
    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"')


def ask_iteration_range() -> tuple:
    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.'
                  ' Please try again.')
        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


print_fizz_buzz_output(ask_iteration_range(),list(ask_divisor_text_pairs()))

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))


def ask_divisor_text_pairs():
    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"')


def ask_iteration_range() -> tuple:
    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


print_fizz_buzz_output(ask_iteration_range(),list(ask_divisor_text_pairs()))

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)


def ask_divisor_text_pairs():
    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"')


def ask_iteration_range() -> tuple:
    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__':
    print_fizz_buzz_output(*ask_iteration_range(), list(ask_divisor_text_pairs()))

Attribution
Source : Link , Question Author : Kaz , Answer Author : Community

Leave a Comment