28
\$\begingroup\$

I just recently started working on python (interesting language, actually), and, as I always do when I learn a new language (with the exception of BrainFuck, that's too hard), I write a FizzBuzz program.

I notice there is no error-checking, so my program would have problems with incorrect input, but please ignore that as I haven't gotten to that part yet.

Code:

def getString(num, fizz, buzz):
    if num % (fizz * buzz) == 0:
        return "FizzBuzz"
    elif num % fizz == 0:
        return "Fizz"
    elif num % buzz == 0:
        return "Buzz"
    else:
        return num

def fizzbuzz(maxNum, fizz, buzz):
    num = 1;
    while num <= maxNum:
        print(getString(num, fizz, buzz))
        num += 1

fizz = int(input("Enter the number to Fizz: "))
buzz = int(input("Enter the number to Buzz: "))
maxNum = int(input("Enter the maximum number: "))
fizzbuzz(maxNum, fizz, buzz)

The code will take a fizz, buzz, and a maxNum as input, and will output, all the fizzing and buzzing the code is doing.

Concerns:

  1. Does it follow Python conventions?
\$\endgroup\$
5
  • 11
    \$\begingroup\$ Note that your logic is actually wrong. getString(4, 2, 4) == "Fizz" when it should actually equal "FizzBuzz". This bug occurs when fizz and buzz are not coprime. \$\endgroup\$
    – Bill Lynch
    Dec 15, 2015 at 4:14
  • 3
    \$\begingroup\$ FizzBuzz in Brainfuck too hard? Madness. \$\endgroup\$
    – Mast
    Dec 15, 2015 at 12:03
  • 1
    \$\begingroup\$ Relevant answer on Code Golf. \$\endgroup\$ Dec 15, 2015 at 17:00
  • \$\begingroup\$ It never hurts to keep things up to par with pylint 's standards. Possibly pep8 's too. \$\endgroup\$ Dec 29, 2015 at 10:38
  • \$\begingroup\$ Note: The bug described by @BillLynch can be avoided by using if num % fizz == 0 and num % buzz == 0: \$\endgroup\$
    – Gloweye
    Oct 9, 2019 at 8:23

6 Answers 6

24
\$\begingroup\$
  • getString and maxNum should be get_string and max_num, by PEP 8 recommendation.
  • getString sometimes returns a number, and sometimes returns a string. This violates the expectation set up by the function name. Also, a function that is indecisive about its return type is harder to work with.
  • num = 1; ← No semicolons in Python, please.
  • The fizzbuzz loop is written idiomatically as for num in range(1, maxNum + 1): …
\$\endgroup\$
5
  • 1
    \$\begingroup\$ Wow, I didn't notice the semicolon, but it runs fine with is there. Is there a reason for that? \$\endgroup\$ Dec 15, 2015 at 1:05
  • 2
    \$\begingroup\$ Python prefers using proper code layout for separating statements. Putting multiple statements on the same line, separated by semicolons, is discouraged. The newline suffices to end a statement (unless it's preceded by a backslash; or is within a unmatched (, {, or [; or it's within a triple-quoted string). So, there is no need to clutter your code with excess punctuation. \$\endgroup\$ Dec 15, 2015 at 1:10
  • \$\begingroup\$ @TheCoffeeCup In case it's still unclear. Python can use semicolons to signal newlines, but you don't need to (unless you're trying to put multiple statements on one line, which is discouraged anyway). \$\endgroup\$ Dec 17, 2015 at 11:31
  • \$\begingroup\$ wouldn't xrange be more idiomatic? \$\endgroup\$
    – chicks
    Dec 21, 2015 at 17:32
  • \$\begingroup\$ @chicks The question is tagged as python-3.x. There is no xrange(). Even if it were Python 2, I would just use range() for compatibility if the range is small and performance is not critical. \$\endgroup\$ Dec 22, 2015 at 5:02
15
\$\begingroup\$

In addition to other answers here is a common way to avoid having too much code on top level:

def main():
    fizz = int(input("Enter the number to Fizz: "))
    buzz = int(input("Enter the number to Buzz: "))
    maxNum = int(input("Enter the maximum number: "))
    fizzbuzz(maxNum, fizz, buzz)

if __name__ == '__main__':
    main()

This patterns allows for your script to be called from the command line, and it also allows for use as an module, so that you reuse your fizzbuzz() function.

And whilst at it, even though this is a rather simple functions, please look into using docstrings to document your function and potentially your module.

\$\endgroup\$
4
\$\begingroup\$

In addition to previous answers:

It is considered more Pythonic to check if not num % fizz*buzz than if num % fizz*buzz == 0

Also you don't need to use elif if there is a return statement inside if, so I'd rewrite your get_string function like this:

def get_string(num, fizz, buzz)
    if not num % fizz*buzz:
        return "FizzBuzz"
    if not num % fizz:
        return "Fizz"
    if not num % buzz:
        return "Buzz"
    return str(n)
\$\endgroup\$
2
  • 1
    \$\begingroup\$ In general I agree that using not something is better than something != 0 or something is not None... When used in conjunction with the modulo operator, I'm a little more sceptic. It kinda reads if not num then take the modulo of the not num afterwards. \$\endgroup\$
    – holroy
    Dec 16, 2015 at 21:33
  • \$\begingroup\$ Will parentheses help to clarify that? \$\endgroup\$ Dec 18, 2015 at 0:54
2
\$\begingroup\$

I notice there is no error-checking, so my program would have problems with incorrect input

Lack of error checking, eg duck typing is generally approved of and preferred in python. Surely there are good cases to use input checking, but generally duck-typing is preferred.

I suspect this preference in python is mostly due to all the "primitives" actually being first class objects: int, str, bool. They all have values, methods, and identities.

Besides, type checking often just gets in the way of your users. Say I use a integer-like class that is tied to an active record or has other features that make it better to work with in my application; a type check is just a barrier to entry that maybe I don't care to hoop-jump for. I use my integer class in place of integers everywhere in my app just fine, without type checking; your code may/would have optimistically run with whatever given object and worked just fine too. Functions that quit just because say an input object doesn't identify as an integer are just plain annoying.

Of course other subtle and clever advantages could be leveraged too. A caller could cleverly integrate with your function in rich ways, passing an impersonated integer-compatible object with valuable performance/analytics tracking and/or whatever else might be needed for a proper unit test, or an object that just knows that when certain maths is applied, that it should update a progress bar. Whatever odd edge case matters, Python so often has an "in". The "we're all consenting adults" dogma that doesn't shield any part of the runtime from good (or bad) ideas, lets the analytics engineer harmlessly track the core components, and lets the performance engineer substitute in-place code with bootstrapping and wrappers that hold over until something can be re-architected.

If you are to pursue type checking I would at most suggest catching the kind of operational arithmetic error that incompatible types would cause and raise instead a type error, or a specific-to-your-module exception that probably extends TypeError, when the input has produced an apparent failure condition.

The value in extending a module-specific exception is that it gives any caller a sure way to catch errors only from your module, without giving up valuable semantics or messages.

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

Always take the "early exit"!

Try to dismiss simple (or error) cases early, "exiting" (returning, raising...) those first, so the rest of the function can avoid thinking about that case anymore.

In practice, it means each if with a return doesn't need an else, reducing the amount of indentation.

def getString(num, fizz, buzz):
    if num % (fizz * buzz) == 0:
        return "FizzBuzz"
    if num % fizz == 0:
        return "Fizz"
    if num % buzz == 0:
        return "Buzz"
    # Only possible if none of the above
    return num
\$\endgroup\$
0
\$\begingroup\$

I have some observations that are mostly language-independent.

Document your functions

A clear description of functionality helps when you're considering re-using code in a new project. Get in the habit of writing good doc-comments early, so you're not caught out trying to unravel your logic when it matters!

Accept command-line arguments

It's tedious to test a program if you always need to supply inputs. Simple numbers are best provided as arguments when invoking the program (consider reading from the input stream as a fallback).

Validate inputs

Does it make sense to accept negative numbers? I don't think so; we should refuse to operate if any of the inputs are zero or less.

Separate computation and I/O

It's hard to unit-test code that produces output using print(). It's easier if we return an collection of values or if we write to a supplied iterator. Python helps us, because it allows us to write a generator simply by using yield to return each value.

Division is the most expensive of the basic arithmetic operations

getString() (which should be get_string() by PEP-8, but really needs to be something more informative) has no knowledge that it is being called with sequential values of num. That prevents a key optimisation that's possible if we inline the function. Then we're able to maintain variables to record the remainders without needing the % operator.


Modified code

Incorporating my suggestions, and some improvements from other answers:

#!/usr/bin/python3
        
import sys

def fizzbuzz(max_num, fizz, buzz):
    '''
    A generator that transforms the range [1..max_num], returning each
    number converted to string, unless it's an exact multiple of
    'fizz' (when it returns the string "Fizz") or of 'buzz' (returns
    "Buzz") - or for multiples of both divisors, the combined string "FizzBuzz".

    >>> list(fizzbuzz(6, 2, 3))
    ['1', 'Fizz', 'Buzz', 'Fizz', '5', 'FizzBuzz']
    >>> list(fizzbuzz(6, 2, 4))
    ['1', 'Fizz', '3', 'FizzBuzz', '5', 'Fizz']
    '''
    # accumulators tracking num % fizz and num % buzz
    fizz_acc = 0
    buzz_acc = 0
    for num in range(1, max_num + 1):
        fizz_acc += 1
        buzz_acc += 1
        result = ""
        if fizz_acc == fizz:
            fizz_acc = 0
            result += "Fizz"
        if buzz_acc == buzz:
            buzz_acc = 0
            result += "Buzz"
        yield result if result else str(num)


def arg_or_input(argno, prompt):
    '''
    Read a value from the 'argno'th command-line argument,
    or if there are fewer, use 'prompt' to input interactively.
    '''
    try:
        return sys.argv[argno]
    except IndexError:
        return input(prompt)


if __name__ == '__main__':
    import doctest
    if doctest.testmod()[0]:
        sys.exit(1)

    try:
        fizz = int(arg_or_input(1, "Enter the number to Fizz: "))
        buzz = int(arg_or_input(2, "Enter the number to Buzz: "))
        max_num = int(arg_or_input(3, "Enter the maximum number: "))
    except ValueError:
        print("Inputs must be integers!", file=sys.stderr)
        sys.exit(1)
        
    if fizz <= 0 or buzz <= 0 or max_num <= 0:
        print("All inputs must be positive!", file=sys.stderr)
        sys.exit(1)

    for s in fizzbuzz(max_num, fizz, buzz):
        print(s)
\$\endgroup\$
1
  • \$\begingroup\$ In many compiled languages, division by a constant can be replaced by a carefully-chosen multiplication, which isn't so bad, but I believe most Python implementations are unable to make that optimisation. Addition is still cheaper (in time and/or electrical energy used). \$\endgroup\$ Jun 9, 2023 at 6:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.